Issue 29775602: Noissue - Update JSDoc to mark optionals in lib/filterClasses.js (Closed)
|
Created:
May 9, 2018, 4:52 p.m. by Manish Jethani Modified:
May 11, 2018, 12:06 p.m. Reviewers:
kzar CC:
sergei Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionNoissue - Update JSDoc to mark optionals in lib/filterClasses.js
Patch Set 1 #
Total comments: 5
MessagesTotal messages: 6
Patch Set 1 I find it useful to know if something is optional, and the documentation should reflect. Now it's confusing because the documentation sometimes is right and sometimes wrong. If we mark something as optional then it should be consistently done all over the place. We could update other files as well but I am working in this part now, I can look at the others later. https://codereview.adblockplus.org/29775602/diff/29775603/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29775602/diff/29775603/lib/filterClasses.j... lib/filterClasses.js:700: * @see ActiveFilter.sitekeys This may seem unrelated but I wanted to specify that the value can be null, but ESLint wouldn't allow me, then I realized the documentation can just be inherited. https://codereview.adblockplus.org/29775602/diff/29775603/lib/filterClasses.j... lib/filterClasses.js:995: * @type {string} Note that selector will always be non-null.
https://codereview.adblockplus.org/29775602/diff/29775603/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29775602/diff/29775603/lib/filterClasses.j... lib/filterClasses.js:995: * @type {string} On 2018/05/09 16:56:08, Manish Jethani wrote: > Note that selector will always be non-null. I guess this is technically incorrect for the abstract ElemHideBase class, since in a few lines you can see it's null.
https://codereview.adblockplus.org/29775602/diff/29775603/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29775602/diff/29775603/lib/filterClasses.j... lib/filterClasses.js:995: * @type {string} On 2018/05/10 11:06:25, kzar wrote: > On 2018/05/09 16:56:08, Manish Jethani wrote: > > Note that selector will always be non-null. > > I guess this is technically incorrect for the abstract ElemHideBase class, since > in a few lines you can see it's null. So ElemHideBase is an abstract class, we don't create instances of it. Assuming that this documentation here is going to be inherited by the derived classes, then it is true that selector will never be null. I don't know if we're actually generating the HTML documentation out of this, and I'm pretty sure there would be a lot of things to fix (which hopefully we will do once we switch to ES2015 classes), but for now at least in the source code I hope that the JSDoc comments reflect how things are.
https://codereview.adblockplus.org/29775602/diff/29775603/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29775602/diff/29775603/lib/filterClasses.j... lib/filterClasses.js:995: * @type {string} On 2018/05/11 03:20:57, Manish Jethani wrote: > On 2018/05/10 11:06:25, kzar wrote: > > On 2018/05/09 16:56:08, Manish Jethani wrote: > > > Note that selector will always be non-null. > > > > I guess this is technically incorrect for the abstract ElemHideBase class, > since > > in a few lines you can see it's null. > > So ElemHideBase is an abstract class, we don't create instances of it. Assuming > that this documentation here is going to be inherited by the derived classes, > then it is true that selector will never be null. > [...] By the way there's a @abstract tag in JSDoc [1], but I think we might be overdoing it if we use it on the code in its current form. Since we're switching to ES2015 classes anyway, I think it would be more useful to mark an entire class as abstract (which would mean all members are "abstract" too). [1]: http://usejsdoc.org/tags-abstract.html
On 2018/05/11 03:20:59, Manish Jethani wrote: > So ElemHideBase is an abstract class, we don't create instances of it. Assuming > that this documentation here is going to be inherited by the derived classes, > then it is true that selector will never be null. Yea fair enough, I think your change is an improvement and like you say we might be switching to proper ES6 classes in the future anyway. LGTM. > I don't know if we're actually generating the HTML documentation out of this We do, you can type `./build.py docs /some/path` to generate them yourself, or take a look at them online at https://adblockplus.org/jsdoc/adblockpluscore. > ...for now at least in the source code I hope that the JSDoc comments reflect how things are. Yea me too, thanks for taking the time to fix these things you notice. 
|
||||||||||||||||||||||||||||||||||||||||||
