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

Fix: Expected order of jsdoc tags (fixes #9412) #9451

Merged
merged 3 commits into from Oct 22, 2017

Conversation

Projects
None yet
4 participants
@Orlandster1998
Contributor

Orlandster1998 commented Oct 16, 2017

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

[ ] Documentation update
[x] 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:

#9412

What changes did you make? (Give an overview)
I've added tests to make sure that the jsdoc tags can be used in any order. So you will find each possible combination of jsdoc tags, based on these tags.

I've also removed the logic from the switch cases to fix the issue. Now the switch cases will just be used to set the status flags. (e.g. hasParams...)

I think it's definitely better to separate the logic from the cases, especially when it comes to different kind of dependencies in between this status flags. (that's why the bug occurred)

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

@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Oct 16, 2017

LGTM

@not-an-aardvark

Thanks for the PR! This generally looks good to me, I just have a few minor suggestions to improve code readability.

Show outdated Hide outdated lib/rules/valid-jsdoc.js
Show outdated Hide outdated lib/rules/valid-jsdoc.js
Chore: Improve code readability (refs #9412)
Create spelling variable names and remove unecessary if statement.
@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot commented Oct 16, 2017

LGTM

@Orlandster1998

This comment has been minimized.

Show comment
Hide comment
@Orlandster1998

Orlandster1998 Oct 16, 2017

Contributor

@not-an-aardvark Thanks for the review. You're absolutely right! I've made a commit with the improvements.

Contributor

Orlandster1998 commented Oct 16, 2017

@not-an-aardvark Thanks for the review. You're absolutely right! I've made a commit with the improvements.

@not-an-aardvark

Looks good to me. Thanks for contributing!

@kaicataldo

This comment has been minimized.

Show comment
Hide comment
@kaicataldo

kaicataldo Oct 22, 2017

Member

LGTM. Thanks for contributing to ESLint!

Member

kaicataldo commented Oct 22, 2017

LGTM. Thanks for contributing to ESLint!

@kaicataldo kaicataldo merged commit b12cff8 into eslint:master Oct 22, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@eslint eslint bot locked and limited conversation to collaborators Apr 21, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.