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:
    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 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 silvenon commented Jan 6, 2018

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

@j-f1
Copy link
Contributor

@j-f1 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 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

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

@platinumazure platinumazure Jan 17, 2018

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

Copy link
Contributor

@j-f1 j-f1 Jan 17, 2018

@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

@platinumazure platinumazure Jan 17, 2018

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

@silvenon silvenon Jan 19, 2018

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
5 checks passed
@not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Jan 20, 2018

Thanks for contributing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants