☁️ Nx Cloud Report

CI is running/has finished running commands for commit2bae913. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Sent with 💌 fromNxCloud.

Thanks for the PR,@sajikix!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day.

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	abb92d8
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/641d34c527f83800089d8e9e
😎 Deploy Preview	https://deploy-preview-5728--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site settings.

Codecov Report

Merging#5728 (abb92d8) intomain (720e811) willincrease coverage by0.05%.
The diff coverage is100.00%.

@@            Coverage Diff             @@##             main    #5728      +/-   ##==========================================+ Coverage   87.24%   87.29%   +0.05%==========================================  Files         383      384       +1       Lines       13080    13139      +59       Branches     3845     3859      +14     ==========================================+ Hits        11411    11470      +59  Misses       1302     1302                Partials      367      367

Sorry for the wait since I got your review!

There have been two major changes since previous review.

1. change duplicate check to recursive ASTNode comparison instead of string comparison fromsourceCode.getText.
2. useremoveRange instead ofreplaceText in the fix function.

1. use recursive ASTNode comparison

As you reviewed, the previous implementation compared pure code strings, so there was a bug that prevented comparison if there were comments, etc. in the union/intersction type.
To resolve this, I changed to compare ASTNode recursively. Specifically, this rule is looking for an exact match for each ASTNode excludingrange,loc, andparent properties. (I first tried to write a comparison process for each AstTypeNode, but there are so many cases that I am not confident I could cover them... 😭 )
I believe this method can be used to compare union/intersection members, but do you have any concerns?

2. use removeRange()

As you reviewed, we are using removeRange instead of replaceText in the fix function. Specifically, I set the removeRange as follows.

typeT=A|A|B;//        ^   ^//     start  end// start : end loc of the member before the duplicated member// end : end loc of the duplicated member// ↓ Deleting this range will result in ....typeT=A|B;

This is a relatively large change, but I would appreciate your review. 🙏

Thank you so much for your detailed explanation!
And I finally realized the problem with my implementation. 🙏

The problem with the current behavior is that there are multiple fixers (number of detected duplicates) that remove the exact same duplicate (the one at the end), right?
And this kind of fixer behavior is not very good for eslint's autofix behavior: .... 😭

Now that I am aware of the problem, I would like to implement it according to the second solution you suggested (= report / fixer only the first duplicates found in constituents).
In addition, I will include a change to correctly specify thenode andloc options in the report function. This will make it easier to see which parts are duplicates and which parts will be removed. (You suggested this too!).

However, I am wondering if the above change (pointing out the first detected duplicate) should be applied to the report function as well, or only to the fixer.
In other words, which of the following is the preferred behavior?

1. report and providefixer only the first duplicate detected

typeF=(A|B)|(A|B)|((C|D)&(A|B))|(A|B);// reported         ^^^^^// fixer            ^^^^^

2. provide a fixer only for the first duplicate detected (report all duplicated)

typeF=(A|B)|(A|B)|(C|D)&(A|B)|(A|B);// reported         ^^^^^     ^^^^^^^^^^^^^^^     ^^^^^// fixer            ^^^^^

☁️ Nx Cloud Report

CI is running/has finished running commands for commitabb92d8. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Sent with 💌 fromNxCloud.

I have addressed the issues regarding the range of the fixer function and the clarity of the error reporting scope!

You can see these improvements in action by checking out theplayground. (It should be fixed properly 🙏 )

Talking about implementation, I have cut thereport function and changed the implementation to call it each time a duplicate is found.

Thank you again and again for your kind review! 🙏
And we are very happy that you will be adding it to the ruleset for v6! 😁

Do I need to do any additional work on this PR? (Is it OK if I leave the rest to you?)

none needed, was just waiting for the build to pass then got distracted 😄