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

Build: Fix JSDoc syntax errors #9813

Merged
merged 2 commits into from Jan 20, 2018
Merged

Build: Fix JSDoc syntax errors #9813

merged 2 commits into from Jan 20, 2018

Conversation

silvenon
Copy link
Contributor

@silvenon silvenon commented Jan 6, 2018

What is the purpose of this pull request? (put an "X" next to item)

  • Documentation update
  • Bug fix (template)
  • New rule (template)
  • Changes an existing rule (template)
  • Add autofixing to a rule
  • Add a CLI option
  • Add something to the core
  • Other, please explain: I fixed the docs script which was failing due to JSDoc syntax errors.

What changes did you make? (Give an overview)

I fixed the docs script that was failing due to JSDoc syntax errors.

  • JSDoc doesn't yet support specifying array content, i.e. "[number,
    number]" is a syntax error, so I'm using "number[]" instead. (source:
    How to document array elements? jsdoc/jsdoc#1073)

  • JSDoc doesn't support multiline objects, e.g. returning report information
    was a syntax error, so I extracted it into a typedef, as well as the
    disable directive.

Is there anything you'd like reviewers to focus on?

  • I added two additional @typedefs, which now show up in the docs. Is that acceptable or should I inline them?
  • The [number, number] syntax isn't a valid way to describe arrays, so I had to turn it into number[], which is less descriptive. I could perhaps extract it into a separate typedef and add a description, but it sounds like an overkill.

@jsf-clabot
Copy link

jsf-clabot commented Jan 6, 2018

CLA assistant check
All committers have signed the CLA.

@silvenon silvenon changed the title Fix JSDoc syntax errors Build: Fix JSDoc syntax errors Jan 6, 2018
@silvenon
Copy link
Contributor Author

silvenon commented Jan 6, 2018

I'm not sure why my commit message is invalid. 😄

@j-f1
Copy link
Contributor

j-f1 commented Jan 6, 2018

I think you have to have a colon (:) after Fix.

JSDoc syntax errors were causing "npm run docs" to fail.

- JSDoc doesn't yet support specifying array content, i.e. "[number,
  number]" is a syntax error, so I'm using "number[]" instead. (source:
  jsdoc/jsdoc#1073)

- JSDoc doesn't support multiline objects, e.g. returning report information
  was a syntax error, so I extracted it into a typedef, as well as the
  disable directive.
@silvenon
Copy link
Contributor Author

silvenon commented Jan 6, 2018

@j-f1 lol, yeah, I added the colon at the wrong place. Now it works, yay. 🎉

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Just left a couple of questions.

* @property {(number|undefined)} endColumn
* @property {(string|null)} nodeType
* @property {string} source
* @property {({text: string, range: (number[]|null)}|null)} fix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this should be extracted to a typedef? It's a bit gnarly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@property {Fix|null} fix maybe?

@@ -121,7 +137,7 @@ function compareFixesByRange(a, b) {
* Merges the given fixes array into one.
* @param {Fix[]} fixes The fixes to merge.
* @param {SourceCode} sourceCode The source code object to get the text between fixes.
* @returns {{text: string, range: [number, number]}} The merged fixes
* @returns {{text: string, range: number[]}} The merged fixes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with JSDoc. Can we do something like number[2] or otherwise represent the ranges as a 2-tuple somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it doesn't work, and I haven't found any details about specifying the length of the array on the internet. I don't think JSDoc supports it.

@not-an-aardvark not-an-aardvark merged commit 4f898c7 into eslint:master Jan 20, 2018
@not-an-aardvark
Copy link
Member

Thanks for contributing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants