Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DMartens
Copy link
Contributor

@DMartens DMartens commented Jun 4, 2022

This fixes #725 by creating a method based on the current isUncommentedExport and checking in iterateJsdoc, so other rules may be added as well.
Currently it adds the option to require-description, require-param-description and require-returns-description.

Notes

  • I disabled the mocha eslint plugin locally for linting as it tries an unsupported deep import from ramda (Node 18)
  • It seems that the export parser does not yet support private identifiers, so all methods of exported classes are public
  • I added the same basic tests for all rules and wanted to write specific tests for export parser but there are currently none?
  • The rule docs refer to the documentation of require-jsdoc. Maybe a new section in the readme should be created?
  • All the rules share the same schema for publicOnly. Maybe this should be shared?

@brettz9
Copy link
Collaborator

brettz9 commented Jun 4, 2022

Thanks for this... Expect to be a bit busy this week, but hopefully can take a look next week or so.

Copy link
Collaborator

@brettz9 brettz9 left a 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),
};
Copy link
Collaborator

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] &&
Copy link
Collaborator

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.

@brettz9
Copy link
Collaborator

brettz9 commented Jun 9, 2022

* I disabled the mocha eslint plugin locally for linting as it tries an unsupported deep import from ramda (Node 18)

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.

* It seems that the export parser does not yet support private identifiers, so all methods of exported classes are public

Ok. I think the export parser could also be improved or have bugs fixed in other ways.

* I added the same basic tests for all rules and wanted to write specific tests for export parser but there are currently none?

Right. If you run npm run test-cov, you can see though that exportParser.js is currently fully covered through our example rules. Specific unit tests would be welcome but not required.

* The rule docs refer to the documentation of `require-jsdoc`. Maybe a new section in the readme should be created?

I'm not personally as concerned where it goes, but if adding the proposed publicOnly setting too, that setting should be mentioned in the main README, with a note that the individual rules also have a publicOnly option.

* All the rules share the same schema for `publicOnly`. Maybe this should be shared?

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.

@DMartens
Copy link
Contributor Author

After thinking about it more I think the best way to solve settings vs options is that rules only have a boolean for publicOnly.
Then pass the value of the setting which will be just set to true if not specifically set.
This would also make sure that every rule uses the same options as it is unlikely that users want to handle public functions differently.
What do you think?

@brettz9
Copy link
Collaborator

brettz9 commented Jun 12, 2022

After thinking about it more I think the best way to solve settings vs options is that rules only have a boolean for publicOnly. Then pass the value of the setting which will be just set to true if not specifically set. This would also make sure that every rule uses the same options as it is unlikely that users want to handle public functions differently. What do you think?

Hmmm... Besides introducing a breaking change for jsdoc/require-jsdoc, I fear some people will try it on a per-rule basis and be disappointed to find it doesn't work (even if I also wouldn't expect anyone would really need to use different per-rule publicOnly config besides the setting default vs. false).

I think I'd prefer we err on the side of greater user control. But we could allow a "default" option which fell back to the setting value.

@brettz9
Copy link
Collaborator

brettz9 commented May 10, 2023

@DMartens : Do you expect to be able to revisit this?

@DMartens
Copy link
Contributor Author

Sorry I lost sight on this. I will finish this at the end of the week at the latest

@brettz9
Copy link
Collaborator

brettz9 commented May 10, 2023

Take the time you need. Just wondering if we should close it, but will keep it open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add publicOnly option to other rules
2 participants