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

feat(eslint-plugin): add config all.json #313

Merged
merged 30 commits into from May 15, 2019

Conversation

Projects
None yet
5 participants
@ldrick
Copy link
Contributor

commented Feb 24, 2019

Fixes: #298
Fixes: #353

This PR fixes possibility to run recreation of shared configs with ts-node and adds all.json as new shared config.

@ldrick

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Don't know, why it fails 🔨
Please can someone have a look and help me finding the issue.
My commits didn't change anything, which should lead to:

[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/3c03136b-737b-4892-bb7e-ac0cee6b5046.sh
yarn run v1.13.0
$ eslint . --ext .js,.ts
Error: Error while loading rule '@typescript-eslint/promise-function-async': You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.
    at Object.getParserServices (/home/vsts/work/1/s/packages/eslint-plugin/dist/util/getParserServices.js:14:15)
    at create (/home/vsts/work/1/s/packages/eslint-plugin/dist/rules/promise-function-async.js:65:37)
    at Object.create (/home/vsts/work/1/s/packages/eslint-plugin/dist/util/createRule.js:15:20)
    at createRuleListeners (/home/vsts/work/1/s/node_modules/eslint/lib/linter.js:577:21)
    at Object.keys.forEach.ruleId (/home/vsts/work/1/s/node_modules/eslint/lib/linter.js:731:31)
    at Array.forEach (<anonymous>)
    at runRules (/home/vsts/work/1/s/node_modules/eslint/lib/linter.js:689:34)
    at Linter._verifyWithoutProcessors (/home/vsts/work/1/s/node_modules/eslint/lib/linter.js:891:31)
    at preprocess.map.textBlock (/home/vsts/work/1/s/node_modules/eslint/lib/linter.js:940:35)
    at Array.map (<anonymous>)
error Command failed with exit code 2.

Resolved 😅

@armano2 armano2 added the enhancement label Feb 26, 2019

@codecov

This comment has been minimized.

Copy link

commented Feb 28, 2019

Codecov Report

Merging #313 into master will increase coverage by 0.29%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #313      +/-   ##
==========================================
+ Coverage   95.52%   95.82%   +0.29%     
==========================================
  Files          89       90       +1     
  Lines        4112     4164      +52     
  Branches     1153     1153              
==========================================
+ Hits         3928     3990      +62     
+ Misses         82       72      -10     
  Partials      102      102
Impacted Files Coverage Δ
packages/eslint-plugin/src/rules/ban-ts-ignore.ts 100% <ø> (ø) ⬆️
.../eslint-plugin/src/rules/promise-function-async.ts 100% <ø> (ø) ⬆️
packages/eslint-plugin/src/rules/unbound-method.ts 96.87% <ø> (ø) ⬆️
...ages/eslint-plugin/src/rules/no-require-imports.ts 100% <ø> (ø) ⬆️
packages/eslint-plugin/src/index.ts 100% <100%> (+100%) ⬆️
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø)
...es/eslint-plugin/src/configs/eslint-recommended.ts 100% <0%> (+100%) ⬆️

ldrick added some commits Mar 2, 2019

@bradzacher

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

A few things:

  • How come you dropped requireindex in favour of using a manually written barrel file? I like that you have a test which will help prevent people from forgetting to do it when adding new rules - just wondering why.

  • IMO instead of hard coding the list of disabled rules in base.json, we should add it as a config into the createRule function. This way it's explicitly defined by the rule itself instead of being an implicit connection we need to remember to maintain.
    @typescript-eslint/core-team let me know if you agree here.

createRule, and its return type is RuleModule

would suggest specifically adding a new property to RuleMetaDataDocs, say

interface RuleMetaDataDocs {
  // ....

  /**
   * Set to true if the rule overrides (i.e. replaces) the base eslint rule of the same name. This will cause the associated base rule to be disabled when generating the shared configs.
   */
  overridesBaseRule?: true
}
  • finally, I would move base.json into the generated configs themselves. If we were writing these configs by hand - I would agree that it's good to follow DRY, but considering that we're generating the configs, IMO this extra layer of indirection just makes it harder to figure out exactly what each of the configs does.
@ldrick

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Hi @bradzacher , thanks for reviewing.

How come you dropped requireindex in favour of using a manually written barrel file? I like that you have a test which will help prevent people from forgetting to do it when adding new rules - just wondering why.

I tried to shortly explain on @armano2 's review, but I see it collapsed due to resolving the request.
When I came over to write tests for all.json, I noticed after looking into requireindex, that it requires the dist/rules not src/rules and I was not happy testing on dist. So I tried to load src/rules, which didn't work, due to requireindex doesn't work with .ts files. Later following that path of dist/rules, I noticed, that requireindex doesn't recognize export.default - which would have made me comment more things. So finally I came to the decision to come up with a way which doesn't do anything implicit (or with lots of comments) and had a look into eslint, which also provides all rules with an index file. I still hope the index.ts is for the best for future maintainers and requires not so much knowledge on how requireindex works. I even tried to provide a requireindexts, which I've written locally, but ended up with using import(), which would have been async and might have broken some things in how dist/rules work. But please let me know, if this is not the intended way, I am willing to change it for the best of maintainers.

IMO instead of hard coding the list of disabled rules in base.json, we should add it as a config into the createRule function. This way it's explicitly defined by the rule itself instead of being an implicit connection we need to remember to maintain.
@typescript-eslint/core-team let me know if you agree here.

createRule, and its return type is RuleModule

would suggest specifically adding a new property to RuleMetaDataDocs, say

interface RuleMetaDataDocs {
  // ....

  /**
   * Set to true if the rule overrides (i.e. replaces) the base eslint rule of the same name. This will cause the associated base rule to be disabled when generating the shared configs.
   */
  overridesBaseRule?: true
}

I'll wait for an answer here and sure I'm willing to change it.
🔜

finally, I would move base.json into the generated configs themselves. If we were writing these configs by hand - I would agree that it's good to follow DRY, but considering that we're generating the configs, IMO this extra layer of indirection just makes it harder to figure out exactly what each of the configs does.

Sure, I'll do that, so every config comes from generate-configs.ts
✔️

ldrick added some commits Mar 7, 2019

ldrick added some commits Mar 15, 2019

@VincentLanglet

This comment has been minimized.

Copy link

commented Apr 18, 2019

@ldrick Hi ! I'd love to use this all config. Keep the good work !
You actually have conflict.

@bradzacher bradzacher requested a review from typescript-eslint/core-team Apr 18, 2019

ldrick added some commits Apr 30, 2019

@bradzacher bradzacher self-requested a review May 13, 2019

@bradzacher
Copy link
Member

left a comment

code LGTM.
one required change with the base config. few nitpicks.

Great work.

Show resolved Hide resolved packages/eslint-plugin/src/configs/base.json Outdated
Show resolved Hide resolved packages/eslint-plugin/tools/generate-configs.ts Outdated
Show resolved Hide resolved packages/eslint-plugin/tools/generate-configs.ts Outdated
Show resolved Hide resolved packages/eslint-plugin/tools/generate-configs.ts Outdated

@bradzacher bradzacher merged commit 67537b8 into typescript-eslint:master May 15, 2019

8 checks passed

Semantic Pull Request ready to be squashed
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project 95.82% (+0.29%) compared to 8c8497c
Details
typescript-eslint.typescript-eslint Build #20190515.2 succeeded
Details
typescript-eslint.typescript-eslint (Primary code validation and tests) Primary code validation and tests succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_10_x) Run unit tests on other Node.js versions node_10_x succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_6_x) Run unit tests on other Node.js versions node_6_x succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_8_x) Run unit tests on other Node.js versions node_8_x succeeded
Details

@ldrick ldrick deleted the ldrick:provide-all-config branch May 16, 2019

@ldrick ldrick restored the ldrick:provide-all-config branch May 31, 2019

@ldrick ldrick deleted the ldrick:provide-all-config branch May 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.