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.

Sign up for GitHub

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

Jump to bottom

Docs: fix doc for no-unneeded-ternary rule (fixes #12098) #12410

Merged
merged 4 commits into from
Oct 22, 2019
Merged

Docs: fix doc for no-unneeded-ternary rule (fixes #12098) #12410

merged 4 commits into from
Oct 22, 2019

Conversation

samrae7
Copy link
Contributor

@samrae7 samrae7 commented Oct 12, 2019 •
edited
Loading

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

[X] 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:

What changes did you make? (Give an overview)
Corrected docs for defaultAssignment option of no-unneeded-ternary rule as per this issue: #12098. Also added extra test cases to show more clearly how the option works.

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

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Oct 12, 2019
@samrae7 samrae7 changed the title update doc for no-unneeded-ternary.md update doc for no-unneeded-ternary rule Oct 12, 2019
@samrae7 samrae7 force-pushed the no-unneeded-ternary-doc-fix branch from 064aa0c to fc6ee97 Compare October 12, 2019 08:09
@samrae7 samrae7 changed the title update doc for no-unneeded-ternary rule Docs: fix doc for no-unneeded-ternary rule (fixes #12098) Oct 12, 2019
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Oct 13, 2019
@samrae7
Copy link
Contributor Author

samrae7 commented Oct 19, 2019 •
edited
Loading

I've realised that I slightly misunderstood the issue so I have a couple of changes to make to this PR. Will do them asap this weekend.Please hold off reviewing for now.

@platinumazure platinumazure added the do not merge This pull request should not be merged yet label Oct 20, 2019
@platinumazure
Copy link
Member

Hi @samrae7, I've added the "do not merge" label to try to avoid an accidental merge on our side. Feel free to ping me or another team member when you're ready for review and we can remove the label at that point. Thanks!

@samrae7
Update: add test cases
a807c24 
- default assignment on right hand side
- default assignment not on right side
- assignment that is not default e.g. x ? 1 : x
@samrae7
Update doc for clarity
7b0e09c
@samrae7
Merge branch 'master' of github.com:eslint/eslint into no-unneeded-te…
acc232f 
…rnary-doc-fix
@samrae7
Copy link
Contributor Author

samrae7 commented Oct 21, 2019

@platinumazure I've updated the PR and it's ready for review. Please can you remove the Do not merge label? Thanks

@ilyavolodin ilyavolodin removed the do not merge This pull request should not be merged yet label Oct 21, 2019
platinumazure
platinumazure approved these changes Oct 21, 2019
View reviewed changes
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.

Looks good, thanks for contributing!

(Also: Wow! That option is kind of misleading as it stands. I think it could be improved- in a future PR and with discussion around the design- by basically renaming it, maybe to something like allowTernaryForDefaultValue, since it's not always part of an assignment. But that's definitely a discussion for another day...)

mdjermanovic
mdjermanovic approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic 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! Thanks for the docs and the additional test cases to clarify the behavior! 👍

@platinumazure platinumazure merged commit e15e1f9 into eslint:master Oct 22, 2019
This was referenced Nov 9, 2019
Open
Open
@snyk-bot snyk-bot mentioned this pull request Nov 21, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Dec 1, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Feb 13, 2020
Merged
@snyk-bot snyk-bot mentioned this pull request Feb 27, 2020
Merged
@a-martynovich a-martynovich mentioned this pull request Mar 10, 2020
Merged
This was referenced Mar 11, 2020
Merged
Open
@snyk-bot snyk-bot mentioned this pull request Mar 26, 2020
Merged
This was referenced Mar 26, 2020
Merged
Merged
Open
This was referenced Apr 3, 2020
Merged
Merged
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 21, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mdjermanovic mdjermanovic mdjermanovic approved these changes

@platinumazure platinumazure platinumazure approved these changes

Assignees
No one assigned
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation rule Relates to ESLint's core rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@samrae7 @platinumazure @mdjermanovic @ilyavolodin