Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

[unnecessary-else] Allowed skipping else if statements via options #4599

Merged
merged 3 commits into from
Apr 16, 2019
Merged

[unnecessary-else] Allowed skipping else if statements via options #4599

merged 3 commits into from
Apr 16, 2019

Conversation

timocov
Copy link
Contributor

@timocov timocov commented Mar 23, 2019

PR checklist

Overview of change:

Added option allowElseIf for unnecessary-else rule to skip checking else if statements (only else).

CHANGELOG.md entry:

[new-rule-option] allow-else-if option for unnecessary-else rule

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Thanks for getting started so quickly on this @timocov! Just a couple of touchups.

src/rules/unnecessaryElseRule.ts Show resolved Hide resolved
src/rules/unnecessaryElseRule.ts Outdated Show resolved Hide resolved
@timocov timocov marked this pull request as ready for review March 24, 2019 20:45
@timocov
Copy link
Contributor Author

timocov commented Apr 2, 2019

@JoshuaKGoldberg I've resolved conflicts and pushed requested changes. Can you take a look please?

@adidahiya
Copy link
Contributor

hi @timocov for future PRs, can you please push additional commits instead of amending and force-pushing? we squash commits at the end of PRs, so this workflow makes it easier to review incremental changes.

@timocov
Copy link
Contributor Author

timocov commented Apr 2, 2019

Hi @adidahiya. My apologies about that. Sure! If you wish I can re-push with old state and added merge commit instead of rebase. Just give some time :)

@timocov
Copy link
Contributor Author

timocov commented Apr 2, 2019

@adidahiya I have a state with old commits and ready to re-push it. Is it ok?

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Rule logic looks great! Just a couple of touchups around the options.

},
},
optionsDescription: Lint.Utils.dedent`
You can optionally specify the option \`"allowElseIf"\` to allow "else if" statements.
Copy link
Contributor

Choose a reason for hiding this comment

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

`"allowElseIf"`

Rules typically create an all-caps const for these option names to reduce the chance of mispelling them anywhere. For example, curlyRule.ts declares OPTION_AS_NEEDED and OPTION_IGNORE_SAME_LINE. Let's switch to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 03b2414

optionExamples: [true],
options: null,
optionsDescription: "Not configurable.",
optionExamples: [true, [true, { allowElseIf: true }]],
Copy link
Contributor

Choose a reason for hiding this comment

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

allowElseIf

TSLint rules use dash-case, so this should be allow-else-if. For example, noAnyRule.ts has const OPTION_IGNORE_REST_ARGS = "ignore-rest-args";.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 03b2414

@timocov
Copy link
Contributor Author

timocov commented Apr 11, 2019

@JoshuaKGoldberg it looks like failed test fails and on master too and doesn't related to the PR.

@adidahiya adidahiya merged commit e36cc17 into palantir:master Apr 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants