mapbox / cloudfriend

Helper functions for assembling CloudFormation templates in JavaScript
ISC License
69 stars 9 forks source link

Improve existing documentation and add copyeditor #97

Closed davidtheclark closed 4 years ago

davidtheclark commented 4 years ago

@rclark for review, please.

davidtheclark commented 4 years ago

I noticed that the options object param was being labeled as optional (not required) because they had default parameters: (options = {}). In every case, the options object is actually required, so I removed the default parameter in https://github.com/mapbox/cloudfriend/pull/97/commits/a534c12a83f27709c8cadf12198b75caf25e5f77.

... And this broke tests. I'll think a little.

davidtheclark commented 4 years ago

The conventional input validation approach in these functions takes that default parameter for granted. And it looks like the hope in documentation.js is that you can use the standard JSDoc @name tag — which we could do, but that turns off all inference so we'd also have to manually add @augments tags, instead of letting documentation.js pick up on the extends keyword. So, a few options:

  1. Keep the default parameter, add @name to every JSDoc comment and @augments to a bunch.
  2. Remove the default parameter, add a new little convention to validate that options exists at all and throw a clear error if not (e.g. if (!options) throw new Error('Options are required'), and change tests to account for that new validation.
  3. Same as (2), but instead of expanding the validation conventions here, switch over to a tool like fusspot.

I think that unless someone is enthusiastic about (3), I'm in favor of (2), and will give that a shot.