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

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

Merged
merged 4 commits into from Oct 22, 2019

Conversation

@samrae7
Copy link
Contributor

samrae7 commented Oct 12, 2019

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 eslint bot added the triage 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 samrae7:no-unneeded-ternary-doc-fix branch from 064aa0c to fc6ee97 Oct 12, 2019
@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
@samrae7

This comment has been minimized.

Copy link
Contributor Author

samrae7 commented Oct 19, 2019

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

This comment has been minimized.

Copy link
Member

platinumazure commented Oct 20, 2019

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 added 3 commits Oct 21, 2019
- default assignment on right hand side
- default assignment not on right side
- assignment that is not default e.g. x ? 1 : x
@samrae7

This comment has been minimized.

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

Copy link
Member

platinumazure left a comment

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...)

Copy link
Member

mdjermanovic left a comment

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
17 checks passed
17 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191021.5 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
wip This PR is no longer a work in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.