-
Notifications
You must be signed in to change notification settings - Fork 39
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
fix: Improve ending commas/semicolon behavior #337
Draft
hugop95
wants to merge
18
commits into
azat-io:next
Choose a base branch
from
hugop95:fix/inline-fixes
base: next
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+863
−102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hugop95
changed the title
fix: makes sorting ignore
fix: makes sorting ignore ending Oct 23, 2024
,
and ;
,
and ;
hugop95
force-pushed
the
fix/inline-fixes
branch
from
October 23, 2024 18:04
79ce0b0
to
2623818
Compare
hugop95
changed the title
fix: makes sorting ignore ending
fix: makes most sorting nodes ignore trailing Oct 23, 2024
,
and ;
,
and ;
hugop95
force-pushed
the
fix/inline-fixes
branch
2 times, most recently
from
October 23, 2024 22:03
60220d9
to
f463d51
Compare
hugop95
changed the title
fix: makes most sorting nodes ignore trailing
fix: Improve ending commas/semicolon behavior
Oct 23, 2024
,
and ;
hugop95
force-pushed
the
fix/inline-fixes
branch
9 times, most recently
from
October 24, 2024 17:07
19060b2
to
69acd61
Compare
…emicolon/comma when inline
hugop95
force-pushed
the
fix/inline-fixes
branch
from
October 25, 2024 06:00
69acd61
to
ab72f86
Compare
hugop95
force-pushed
the
fix/inline-fixes
branch
from
October 25, 2024 06:08
ab72f86
to
2b266b8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #336.
Very little code changes outside of tests:
Affected rules
sort-classes
sort-interfaces
sort-object-types
sort-imports
sort-exports
Description
Today, the way commas and semicolons are handled for a given node actually depends on multiple factors:
sort-objects
will never include,
, while a node insort-interfaces
will. This is how the AST parser works. This means that rules that natively ignore commas and semicolons are not affected (sort-enums
for example).sort-object-types
andsort-interfaces
, nodes will include;
and,
.sort-object-types
, commas and semicolons are ignored for computing the length of a node with theline-length
sorting type, but not insort-interfaces
.eslint-plugin-perfectionist/rules/sort-object-types.ts
Lines 252 to 260 in b2afcfd
The last point is problematic, because it fixes a problem in very specific cases (and not the one mentioned in the issue, for example).
Proposal
This PR does the following:
line-length
sorting type always ignore ending semicolons and commas for all rules, similarly to whatsort-object-types
does today.How to know if we need to add a semicolon to a node after sorting
Let's say that a node B needs to take the place of a node A after sorting.
First, B should indicate whether it requires an ending semicolon if inline with a following node.
In
sort-classes
for example,properties
require an ending semicolon if inline, whilenon-abstract methods
don't.If B requires an ending semicolon, we should check whether if ends with a semicolon (or a comma) or not (as that semicolon will be brought with it when moving).
If B does not end with a semicolon/comma, check whether A (the place that B will take) has at least one inline token (other than a semicolon/comma) after it. If it does, this means that we should add a semicolon after B for safety.
There are cases where this process adds a semicolon when it is not actually necessary, but this does not break compilation and doesn't affect runtime. Furthermore, this is likely to be autocorrected by another ESLint rule and is easy to implement, so that's a quick-win in my book.
Also, let's not forget that although these bugs can be very problematic (breaking compilation/runtime), they are not likely to be reproduced that often in reality due to all the conditions required for those bugs to appear, so let's keep it not too complicated!
sort-classes
: Types of nodes and whether they require an ending semicolon or not when inline with a following nodeExamples
Example 1
❌
✅
Example 2
❌
✅
Consequences
sort-object-types
had its output fixed (See commit n°2)!Additional changes
sourceCode.getText()
rather than slicing.Remaining to do
Add inline tests for all rules! (even the ones not affected by #336)
sort-array-includes
sort-classes
No separatorsort-decorators
sort-enums
sort-exports
sort-heritage-clauses
sort-imports
sort-interfaces
sort-intersection-types
No separatorsort-jsx-props
sort-named-exports
sort-named-imports
sort-object-types
sort-objects
sort-sets
sort-switch-case
(Not sure about that one)sort-union-types
sort-variable-declarations
What is the purpose of this pull request?