-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): Begin rewrite of indent rule #439
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
Conversation
@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? |
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. |
@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 |
# Conflicts: # .eslintrc.json # .vscode/launch.json # yarn.lock
Codecov Report
@@ 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
|
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
any
s), 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:
indent
.