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): Begin rewrite of indent rule #439

Merged
merged 6 commits into from May 9, 2019
Merged

Conversation

bradzacher
Copy link
Member

I wanted to commit this into the repo, as the PR was already HUGE as it was.

My existing strategy of spoofing estree nodes and relying on eslint's underlying indentation logic for those nodes just wasn't working well, and really isn't maintainable - it's really dodgy in its typings (lots of anys), and it was lazy in its spoofing for simplicity.
Also there are things that eslint doesn't indent in the way that our users expect (i.e. #121 - this is essentially just a binary expression, but eslint doesn't lint the indent of binary expressions, trust me - I tried to make it work).

So I'm kicking things up a notch. Almost everything in this PR is copied directly from the eslint repo, except it has been adapted to be fully typed, and work within our tooling.
I deleted a few tests because they were asserting eslint's handling (or purposeful not handling) of unknown nodes (i.e. TS-ESTree custom nodes).

I've added the code as a new rule, and for now it's called indent-new-do-not-use. I have not added this rule to the export, so it's not directly available to consumers.

The plan will be as follows:

  1. Merge this PR.
  2. Add in handling for typescript nodes (over the course of several PRs).
  3. Patch handling of the base indent functionality where it makes sense (to handle typescript conventions).
  4. Do the ol' switcheroo; delete the old rule, and rename the new version to indent.
  5. ???
  6. Profit 💰💰💰

@bradzacher bradzacher added this to the indent rewrite milestone Apr 18, 2019
@bradzacher bradzacher added the enhancement New feature or request label Apr 18, 2019
@bradzacher bradzacher requested a review from a team April 18, 2019 16:43
@JamesHenry
Copy link
Member

@bradzacher Sorry if I'm being dumb, but what do we gain by merging this when it's unfinished?

Wouldn't using this as the base branch for follow up PRs be just as effective?

@bradzacher
Copy link
Member Author

It wouldn't quite work the same - once this is merged in, then everything is part of master, meaning that if we were to do any sweeping changes to the codebase (such as #425), I wouldn't have to manually merge them into the working branch.

Also keeping it in a separate branch means that should any modifications made to the same files in master there would be merge conflicts, requiring manual merges.

@JamesHenry
Copy link
Member

@bradzacher Cool, given we are all working as and when we can on this, I think sparing you the ongoing pain of rebasing etc is important, so I'm on board with merging in a rule with known issues, as it is clearly marked as do not use

# Conflicts:
#	.eslintrc.json
#	.vscode/launch.json
#	yarn.lock
@codecov
Copy link

codecov bot commented May 9, 2019

Codecov Report

Merging #439 into master will increase coverage by 0.34%.
The diff coverage is 99.09%.

@@            Coverage Diff             @@
##           master     #439      +/-   ##
==========================================
+ Coverage   95.88%   96.23%   +0.34%     
==========================================
  Files          79       83       +4     
  Lines        3625     4067     +442     
  Branches     1020     1149     +129     
==========================================
+ Hits         3476     3914     +438     
- Misses         51       52       +1     
- Partials       98      101       +3
Impacted Files Coverage Δ
packages/eslint-plugin/src/util/misc.ts 80.95% <ø> (ø) ⬆️
...n/src/rules/indent-new-do-not-use/OffsetStorage.ts 100% <100%> (ø)
...lugin/src/rules/indent-new-do-not-use/TokenInfo.ts 100% <100%> (ø)
...rc/rules/indent-new-do-not-use/BinarySearchTree.ts 100% <100%> (ø)
.../typescript-estree/src/ts-estree/ast-node-types.ts 100% <100%> (ø) ⬆️
packages/eslint-plugin/src/util/applyDefault.ts 100% <100%> (ø) ⬆️
...nt-plugin/src/rules/indent-new-do-not-use/index.ts 98.92% <98.92%> (ø)
... and 1 more

@bradzacher bradzacher merged commit 6eb97d4 into master May 9, 2019
@bradzacher bradzacher deleted the rewrite-indent branch May 9, 2019 17:08
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants