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

Proposal: require more complete JSDoc documentation #8819

Closed
danswick opened this issue Sep 28, 2019 · 5 comments
Closed

Proposal: require more complete JSDoc documentation #8819

danswick opened this issue Sep 28, 2019 · 5 comments

Comments

@danswick
Copy link
Contributor

The plurality of the feedback the docs team has been receiving since launching @mapbox/docs ' feedback module has pertained to the Mapbox GL JS docs. Much of that feedback has been about missing parameters, return values, or descriptions in our API documentation generated from JSDoc comments. I would like to propose adding some stricter tests for requiring JSDoc descriptions, parameters, and return values for all public methods.

Testing / linting

There's a handy and popular ESLint plugin for checking various JSDoc parameters. One benefit of an ESLint plugin is integration with text editors so GL JS contributors can get feedback while working in the codebase.

Establishing a baseline

In order to get a sense for the scope of work required to get Mapbox GL JS to a "complete" state where JSDoc is relatively complete and linting/tests pass, is set up a working branch here: https://github.com/mapbox/mapbox-gl-js/compare/eslint-jsdoc?expand=1 with the following options configured:

"jsdoc/check-param-names": 1,
"jsdoc/require-jsdoc": 1,
"jsdoc/require-param": 1,
"jsdoc/require-param-description": 1,
"jsdoc/require-param-name": 1,
"jsdoc/require-returns": 1,
"jsdoc/require-returns-description": 1

In its current form, my test branch has about 400 JSDoc issues, many of which are for missing JSDoc comments for non-public methods. I could see a few potential paths forward:

  1. For the missing JSDoc warnings, add minimal JSDoc comments for every missing instance flagged by ESLint and mark all as @private (there is an option to ignore all methods marked as private). This option would allow us to be strict about requiring documentation everywhere without requiring a tremendous amount of effor up-front. While my personal opinion is that even private methods should be well-documented, I'm not sure if option is valuable without more detailed descriptions, which would be quite difficult for a causal contributor (🙋‍♂️) to add for many internal methods.
  2. Don't require JSDoc comments everywhere, but where they do exist, require params, return values, and descriptions for each. If we assume all public methods already include some form of JSDoc (I believe this is true), we can start by ensuring our existing documentation is complete and consider expanding to all methods some time in the future. Under this scenario, we'd have about 150 issues to solve, compared with over 400. Some of these issues are already resolved in @colleenmcginnis' series of feedback PRs (ex [docs] Address user feedback for /api: setStyle, getStyle, isStyleLoaded #8807).

Once we've gotten some feedback from the core GL JS team, I'd be happy to prepare a branch with 1) ESLint configuration and 2) many or all of the changes required to get us to a baseline (I could more conceivably do this work if folks want to go with option 2).

@danswick
Copy link
Contributor Author

danswick commented Oct 3, 2019

Just following up here @mapbox/gl-js. After sitting with this for a few days, I am leaning strongly toward option 2. It will be less work to get to a solid baseline, the work will be focused more directly on improving public-facing documentation, and, unlike option 1, it doesn't require extra code comments whose purpose is exclusively to satisfy the linter. Unless folks have issues with this direction, I'd like to get started on option 2 next week.

@ryanhamley
Copy link
Contributor

I really like this. I agree that eventually our goal should be to have documentation for every method, public or private. It's hard to understand some of the internal methods so we end up digging through old Github issues to figure out what they're doing. Having @private docs for them would be helpful for us as well as potential contributors.

But option 2 seems like a good way to get started. Perhaps once we have full documentation for the public methods we can just move on to the private ones. That also has the benefit that we can get the public documentation fixed up for users ASAP without waiting on resolving documentation for a bunch of private methods. Thanks for working on this!

@andrewharvey
Copy link
Collaborator

It's hard to understand some of the internal methods so we end up digging through old Github issues to figure out what they're doing. Having @Private docs for them would be helpful for us as well as potential contributors.

100% agree.

@asheemmamoowala
Copy link
Contributor

Have you tried manually curating a list of the files/folders that are exposed to the public SDK and only enabling the jsdoc linter on those? I started with ["src/ui/**", "src/index.js", "src/style-spec/bin"], but there are a few more that would need to be included.

It can be easy to forget to add something to this list, so I would be in favor of your option 2 until files (or their contents) are sorted into public/private.

cc @mapbox/gl-js

This was referenced Nov 9, 2019
@danswick
Copy link
Contributor Author

This is done!

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

No branches or pull requests

4 participants