-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
feat: Require description only for public functions #890
base: main
Are you sure you want to change the base?
feat: Require description only for public functions #890
Conversation
Thanks for this... Expect to be a bit busy this week, but hopefully can take a look next week or so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the minor changes suggested by the comments...
esm: Boolean(opt?.esm ?? true), | ||
initModuleExports: Boolean(opt?.cjs ?? true), | ||
initWindow: Boolean(opt?.window ?? false), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this same option normalization is needed elsewhere in require-jsdoc
, let's have both reference a new utility (I think just use it also within isUncommentedExport
).
@@ -962,6 +963,14 @@ const iterate = ( | |||
return; | |||
} | |||
|
|||
if ( | |||
context.options[0] && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could also support a check for settings.publicOnly
here in case the publicOnly
setting is used in place of an option.
Could you report this? This is a pretty well-used plugin, and it may be possible they get a fix out in due order if they know about it.
Ok. I think the export parser could also be improved or have bugs fixed in other ways.
Right. If you run
I'm not personally as concerned where it goes, but if adding the proposed
Welcome to add a shared schema utility. It can make the schema more difficult to browse, but it does reduce duplication. I'm kind of ambivalent about it, especially since I'm not as worried about maintaining duplicate JSON as I would be with duplicated JavaScript. |
After thinking about it more I think the best way to solve settings vs options is that rules only have a boolean for |
Hmmm... Besides introducing a breaking change for I think I'd prefer we err on the side of greater user control. But we could allow a |
@DMartens : Do you expect to be able to revisit this? |
Sorry I lost sight on this. I will finish this at the end of the week at the latest |
Take the time you need. Just wondering if we should close it, but will keep it open. |
This fixes #725 by creating a method based on the current
isUncommentedExport
and checking initerateJsdoc
, so other rules may be added as well.Currently it adds the option to
require-description
,require-param-description
andrequire-returns-description
.Notes
require-jsdoc
. Maybe a new section in the readme should be created?publicOnly
. Maybe this should be shared?