-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add Analyzer and CodeFix for duplicate method aliases (ORLEANS0011) #8662
Conversation
Note, this PR is based on #8643 (which has not yet been merged), thats why it contains the |
@ReubenBond If "Fix for Document" than yes, it changes to 'Echo1' and 'Echo2' But if you change them 1-by-1 its actually quite the problem, let me explain why: Thats why if the user applies the code fix for the document, and there are multiple "Echo"s it will go 1, 2, 3. But if the above is true, than we look the only "true" duplicate which would be "Echo" and assign the next number which would be "Echo1" We need to always think that the user's may always name the alias themselves i.e: |
In your example, I would expect the result to be "Echo1". If there was a method with the alias "Echo1" already, then I would expect the result to be "Echo2", and so on until there is no conflict. The method name does not matter (the developer could have aliased a method named GetFoo as "Echo" and the result should be the same), but the resulting alias value does. |
"The method name does not matter" - I see what you mean, i'll address it 👍 |
@ReubenBond Maybe it makes sense to hold off on approving the other PRs which are based on this (since some common code is shared), until this is resolved and i'll port the change to the other 2 PRs. You could still review and leave comments ofc. |
@ReubenBond It should work as expected now! I've ported the fix to the other PRs too, so feel free to continue when you can |
@ReubenBond are we good with this, so we can move onto the other? |
5f60197
to
f52ca9f
Compare
I am tweaking the suffix incrementing code a little, but otherwise LGTM |
d42817d
to
d93091a
Compare
@ledjon-behluli if you can rebase, I'm ready to merge. |
99adcef
to
fc51630
Compare
@ledjon-behluli I performed a squash merge + force push of your latest, since it included changes from main which made final review harder. |
fc51630
to
117735b
Compare
@ReubenBond whatever it takes, stuff got mangled up |
117735b
to
4335591
Compare
cc @ReubenBond
close #8661
Analyzer:
Fixer:
Tests:
Microsoft Reviewers: Open in CodeFlow