Skip to content

feat(eslint-plugin): add "enum-const-style" rule #290

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

Closed
wants to merge 8 commits into from

Conversation

mohsen1
Copy link
Contributor

@mohsen1 mohsen1 commented Feb 16, 2019

Closes #280

@mohsen1 mohsen1 force-pushed the mohsen1--enum-style branch from 0c94702 to 69b371e Compare February 16, 2019 21:53
@mysticatea
Copy link
Member

Thank you for your contribution!

I'd like to separate rules, and in order to separate, would you change the rule name as narrowing?

#280 (comment)

@mysticatea mysticatea changed the title Add "enum-style" rule feat(eslint-plugin): add "enum-const-style" rule Feb 18, 2019
@mysticatea
Copy link
Member

Thank you for update!

Code and tests look good to me.
It will be better if the document described the advantage for both settings. Currently, I feel unfair because the document doesn't describe the pros of const enum.

And would you reorder the document content a bit?

# title (rule ID)

Description of context why you use this rule.

## Rule Details

Description of what the rule does.

## Options

Description of options.

@mohsen1 mohsen1 force-pushed the mohsen1--enum-style branch 2 times, most recently from 4c5d2cb to 24042d8 Compare February 20, 2019 18:17
@mohsen1
Copy link
Contributor Author

mohsen1 commented Feb 20, 2019

I don't get any coverage errors locally. How can I re-run it?

@armano2
Copy link
Collaborator

armano2 commented Feb 20, 2019

depends on editor, in Intelij Idea i'm just pressing
image

for vscode you will need plugin https://github.com/jest-community/vscode-jest#how-do-i-show-code-coverage

@bradzacher
Copy link
Member

if you cd into the plugin folder (rather than just in the project root), then run yarn test it should run the tests for just the plugin, and then dump the coverage to the console.

@mohsen1 mohsen1 force-pushed the mohsen1--enum-style branch from 25dd589 to 4b1292a Compare February 24, 2019 03:43
@mohsen1
Copy link
Contributor Author

mohsen1 commented Feb 24, 2019

I don't see the failure report in codecov. Can you point me to what's actually not passing?

@bradzacher
Copy link
Member

@mohsen1 #290 (comment)

We have a minimum target of 90% coverage on the files that a PR changes.
In particular you only have 87.5% coverage over your new file, which means that you need to look at your branches and make sure you've covered them with your tests.

It's because your file is so short, so one line really messes up the total %age.
Add a valid test case for the 'never' option?

@bradzacher
Copy link
Member

One piece of feedback for you with your PRs - don't use the ammend + force push workflow when submitting PRs.

Doing so makes it hard for us to review because git/github doesn't track the old versions of the same commit, which means that we can't compare your new changes to the previous changes.

i.e. we have to re-review the entire PR each time you push, because we don't know what has changed.

@mohsen1 mohsen1 force-pushed the mohsen1--enum-style branch from c386cdb to 3715b5f Compare March 8, 2019 14:30
@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Mar 15, 2019
@JamesHenry
Copy link
Member

@mohsen1 when do you think you’ll have chance to review the feedback?

@JamesHenry JamesHenry added the awaiting response Issues waiting for a reply from the OP or another party label Apr 4, 2019
@JamesHenry
Copy link
Member

Hi @mohsen1, just following up on this again as it's been a few months since the last comment

@bradzacher bradzacher added the help wanted Extra attention is needed label Nov 22, 2019
@armano2 armano2 requested a review from bradzacher January 19, 2020 21:28
@bradzacher bradzacher removed awaiting response Issues waiting for a reply from the OP or another party help wanted Extra attention is needed labels Jan 21, 2020
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

one change with the docs, otherwise looking good.

Examples of **incorrect** code for this rule with `always` config:

```ts
enum const Foo {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enum const Foo {
const enum Foo {

Comment on lines +11 to +57
### Default config: `never`

```JSON
{
"enum-const-style": ["error", "never"]
}
```

Examples of **incorrect** code for this rule with `never` config:

```ts
enum Foo {
ONE,
TWO,
}
```

Examples of **correct** code for this rule with `never` config:

```ts
const enum Foo {
ONE,
TWO,
}
```

### `always` config

Only const enums are allowed

Examples of **incorrect** code for this rule with `always` config:

```ts
enum const Foo {
ONE,
TWO,
}
```

Examples of **correct** code for this rule with `always` config:

```ts
const Foo {
ONE,
TWO,
}
```
Copy link
Member

Choose a reason for hiding this comment

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

I think the examples are the wrong way around

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually i haven't read docs, but i did small refactoring in rule source code

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jan 21, 2020
@bradzacher bradzacher added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Apr 13, 2020
@bradzacher bradzacher closed this Apr 13, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party enhancement: new plugin rule New rule request for eslint-plugin stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: Const enum
5 participants