-
-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add exported functions option to require-jsdoc rule #262
Conversation
0b64389
to
322307f
Compare
There are at least following limitations:
|
I'm not sure what "branches" are in this context (unless it refers to blocks within a scope?), From my eyeballing, it looks like this will be quite useful for many projects... |
I've amended my comment, as I was forgetting this is for Node modules |
With branches I meant to refer conditional branching, such as statically analyzable if blocks, which are possible in CommonJS. One such rare case would be a polyfill module where a shim would only be returned if the class/method is not already available. I would consider it ok that for such classes or methods the comments are not enforced, since it can still offer implementation with no JSDoc comment. |
51b4d8c
to
80c3cb4
Compare
Noticed some more cases where referencing unknown variables would cause errors to be thrown in exportParser.js. The "browserEnv" option also modifies the window variable when the option is not enabled. I'll submit fixes for these today. |
As nice as it is to be able to analyze the whole file, if it wouldn't be too hard to add an option to only check ancestors, I think that would be a nice optimization and possibly the only needed use case for some usage scenarios. |
Very useful PR. Sorry, I don't know how eslint and its plugin works internally, but I see you are using a custom parser, will that be a problem for Typescript? Would your rule include dangling functions, interfaces, enums, etc Besides, would your solution worked has it used to on tslint completed-docs rule ? I always got frustrated with this rule because it does not allow precise control on where you do expect documentation and where you don't. For example, how to require documentation on everything on interfaces but to restrict the check to the top level doc for classes (no doc required on methods). |
It is definitely useful to consider how the same rule could be made available for both JavaScript and TypeScript. The typescript-eslint project aims at being able to use many of the same ESLint rules with TypeScript. One obvious option is to implement the same rule in that project separately. Another option is to modify this implementation to make extendable, but I'm not sure if that is reasonable approach. |
6a233c9
to
ddbc42e
Compare
Are we ok to merge this as it is now, @Extersky ? |
78c9301
to
43477e0
Compare
f511818
to
af03126
Compare
It'd be nice to get this merged, as I think it should be ready. Anything else @Extersky before sending it on to gajus? |
Sorry, I missed this. From my side merging this is ok in case the optimisation option for only checking through ancestors isn't needed yet. |
The added documentation mentions browser window global export as an option, but this is not yet supported by browserEnv option. Should this be added? This is a simple modification to also check the window variable. I'll start preparing this in case it is included. |
browserEnv option now causes exports to be checked from window global variable. This doesn't yet populate window variable from GeneratorDeclarations, but this isn't currently supported in the rule at all. |
Yeah, sorry, I meant "checks" in the docs, but your docs for it are even better. This looks great now. I think it is ready to merge. Though it may be helpful if you could offer a summary of any remaining to-dos for the lesser features that are not yet implemented, e.g., checking generator declarations. @gajus , as a new feature with changes since your approval, do you still want notice? Don't mean to be annoying, just presume it is better to err on the side of caution until I have a better idea of what to pass on, as I took your earlier comment as being about smaller fixes and this is part of a new feature. |
* When enabled, require all exported function bodies to have JSDoc block * Fixes gajus#192
* Class declarations were not checked * Added guards symbol exports, which caused errors before class declaration handling was added
* Also update README for changes from previous commit
* Change property name from publicFunctionsOnly to publicOnly * The option was changed earlier to also check class declarations
* also generated fixes for readme assertions with allowOverrideWithoutParam
* window variables were not checked for exports * window variable was populated incorrectly
I have no issues with you taking the lead. All changes you've pushed that I have reviewed did not ask for a single comment. Just take the lead and ping me if you need any help. I only ask not to do major code structure changes without first raising an issue (such as rewrite in TS, changing ESLint preset, etc). |
Great, sounds good, thanks! |
- Testing: Add more tests checking difference between `esm` and `cjs` options - Testing: Ensure set other property to `false` to avoid potential interference
@Extersky : I've made one other change (discussed off list), re: switching to "esm", "cjs", and "window" for greater clarity on how the export types apply, as "exports" can, as you suggest, be confusable with |
🎉 This PR is included in version 8.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@Extersky : I just went through the codebase to try to get us to 100% coverage, and though I got through everything else, including some changes to Toward this end, I added an npm script, |
This PR adds support for configuring require-jsdoc rule to require JSDoc comments for exported function bodies only. There are options for configuring the export checks for CommonJS and ES6 exports.
Fixes #192