-
Notifications
You must be signed in to change notification settings - Fork 56
QuickFix: RemoveUnusedNamedPattern #947
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
base: main
Are you sure you want to change the base?
QuickFix: RemoveUnusedNamedPattern #947
Conversation
There are still some unrelated test failing. So i would wait until this is fixed in main. Based on this comment #945 (comment) |
# Conflicts: # ReSharper.FSharp/test/src/FSharp.Intentions.Tests/src/QuickFixes/AddMissingSeqFixTest.fs
# Conflicts: # ReSharper.FSharp/test/src/FSharp.Intentions.Tests/src/QuickFixes/AddMissingSeqFixTest.fs
@auduchinok Any clues on why all the related test in the CI failed ? with
I can see this working locally ![]() |
There may be an error the prevents FCS from reporting the unused pattern one. The test environment is somewhat different from a real project, even though we try to make it similar enough. I should try running it myself to tell more. |
@edgarfgp Could you please rebase on |
@auduchinok Done. No conflicts though. |
ReSharper.FSharp/src/FSharp/FSharp.Psi.Intentions/src/QuickFixes/RemoveUnusedNamedPatternFix.fs
Outdated
Show resolved
Hide resolved
...er.FSharp/test/src/FSharp.Intentions.Tests/src/QuickFixes/RemoveUnusedNamedPatternFixTest.fs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi.Intentions/src/QuickFixes/RemoveUnusedNamedPatternFix.fs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi.Intentions/src/QuickFixes/RemoveUnusedNamedPatternFix.fs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi.Intentions/src/QuickFixes/RemoveUnusedNamedPatternFix.fs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi.Intentions/src/QuickFixes/RemoveUnusedNamedPatternFix.fs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi.Intentions/src/QuickFixes/RemoveUnusedNamedPatternFix.fs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi.Intentions/src/QuickFixes/RemoveUnusedNamedPatternFix.fs
Outdated
Show resolved
Hide resolved
ReSharper.FSharp/src/FSharp/FSharp.Psi.Intentions/src/QuickFixes/RemoveUnusedNamedPatternFix.fs
Outdated
Show resolved
Hide resolved
if isNotNull trailingComment then trailingComment.PrevSibling | ||
else | ||
if fieldPat.EndLine < nextFieldPat.StartLine then | ||
let beforeNext = getFirstMatchingNodeBefore isInlineSpaceOrComment nextFieldPat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch only executes when the next pattern is on another line. isInlineSpaceOrComment
won't go through line separators, so it would only cover some tokens on the next line. Why do we check whitespace/comments on another line when deleting a pattern on the current line?
Why not go to subsequent nodes from the pattern that is being deleted instead? And have you had a chance to test the formatter, as suggested previously? Maybe it can do it all for you after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we check whitespace/comments on another line when deleting a pattern on the current line?
Because I have not found a better way to deal with cases like:
Before
match x with
| { F1 = {caret}f1
// I'm a comment
F2 = f2 // I'm a comment
F3 = {caret}}f3} -> ()
After
match x with
| { // I'm a comment
F2 = f2
F3 = f3} -> ()
And have you had a chance to test the formatter, as suggested previously? Maybe it can do it all for you after all?
No. Assuming that you are referring to .FormatNode()
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Assuming that you are referring to .FormatNode() function
No, I meant just deleting the meaningful pattern nodes, expecting all the newlines and whitespaces to be handled by the formatter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, yeah, it doesn't handle this new line yet. I'll need to add some rules to the formatter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Looking forward to finish this PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@auduchinok Anything I can help with to unblock this PR ?
f683b20
to
dc60995
Compare
Superseeds #463