cliff-effects icon indicating copy to clipboard operation
cliff-effects copied to clipboard

Make JSDoc comments more useful

Open ethanbb opened this issue 7 years ago • 5 comments

Currently, there are several places in the (now publishable) JSDoc comments where:

  • Things are missing (or "TODO")
  • Things that shouldn't be documented (minor local variables) are documented
  • A comment refers to the wrong entity
  • There are (apparently) several entries on the same object, or objects with the same name

Let's clean these up so that the docs make sense!

ethanbb avatar Aug 15 '18 01:08 ethanbb

This sounds great! This would be a great place to take note of those locations when we spot them.

Regarding the documentation of minor variables - I remember doing that a couple of times when I knew less about jsdoc and it seemed appropriate to clarify something. Maybe we can just change those to regular comments if they're still relevant.

knod avatar Aug 19 '18 00:08 knod

I'd be interested in taking this on, but have some questions (I've never used JSDoc before, but I just read up on some on their documentation and feel ready to give this a try, with a little guidance):

  1. How do I generate the actual documentation from the comments, so that I can see what the current result looks like and what the problems with it are? Do I just need to download the JSDoc tool locally and manually generate the current documentation? Or is there a fancier way?
  2. Are we using the latest version of JSDoc/can I use it in writing description comments? I'm wondering since some of the useful-seeming features of JSDoc 3 (ie support for parsing ES 2015 classes) appear to have support only in relatively recent versions (3.4+).
  3. Is it accurate that all classes and all functions in all of the JS files in the whole cliff-effects repo should ideally have JSDoc description comments?
  4. Is there anything besides classes and functions that should get JSDoc description comments?

skudbucket avatar Aug 27 '18 19:08 skudbucket

That would be awesome! Thanks, @skudbucket! I'll answer what I can, and leave the rest to @ethanbb :

  1. @ethanbb should also weigh in on other solutions because I don't quite understand why the docs don't appear locally, but if nothing else works, and you're working on your fork, not our repo, you can do npm run deploy and it should create a github page for your repo where you can see the docs page. Other commands, if you're interested in them, are also in 'package.json'.
  2. @ethanbb
  3. My opinion is that it's not always necessary. If something seems so self-explanatory that the documentation is just redundant, it doesn't need a description. If you're not sure and concerned, though, feel free to add one! Or to ask for input from others, if you're more comfortable with that.
  4. First off, just getting the functions and classes is a great start, and there's plenty of them. If we get to a point where we're worrying about that, I think I'll throw us a party :) Second, to answer teh question as best I can - class methods could use descriptions sometimes. I'm not sure about the convention for anything else.

knod avatar Aug 27 '18 22:08 knod

  1. To see the documentation locally, you can just run npm run docs, which will generate documentation in the top-level docs folder. Then you can open docs/index.html in your browser. The /docs route to view the docs as part of the app only works in production currently because it relies on copying the docs folder to the build folder after building (which I just manually inserted into the build script). I'm not exactly sure how the development server works, but it may be possible to get it to work with that by modifying the start command somehow.

  2. We're using the npm package documentation to generate docs, currently at version 8.0.2. It's not entirely clear when looking at their repo what version of the JSDoc standard they're using, but on the FAQ it says they have "robust ES6 support" etc., so I'd just try using the new features you're looking at and see if they get interpreted correctly.

Thanks for the help!

ethanbb avatar Aug 27 '18 23:08 ethanbb

Started working on this in PR #736.

People should let me know if there's anything they'd like me to change in the way I'm going about it, otherwise I'm going to just start working my way through and documenting all the components and functions in the same manner as I've done App.js in that PR. Feedback appreciated, especially early on when it's really easy to change course without redoing much work! Thanks!

skudbucket avatar Sep 06 '18 15:09 skudbucket