-
Notifications
You must be signed in to change notification settings - Fork 789
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
AddExplicitReturnType refactoring #16077
AddExplicitReturnType refactoring #16077
Conversation
@dotnet-policy-service agree |
@SebastianAtWork that's a great start! Here are a few quick tips for you:
And special thanks for testing efforts! |
9983cbc
to
95ef481
Compare
vsintegration/tests/FSharp.Editor.Tests/Refactors/RemoveExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
vsintegration/src/FSharp.Editor/Refactor/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
407572f
to
92ee24e
Compare
@psfinaki Im currently getting a build error after updating my branch : E:\Repos\fsharp\src\Compiler\Utilities\Cancellable.fs(12,5): error FS0902: For F#7 and lower, static value definitions |
vsintegration/tests/FSharp.Editor.Tests/Refactors/RemoveExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
vsintegration/src/FSharp.Editor/Common/InternalOptionBuilder.fs
Outdated
Show resolved
Hide resolved
@SebastianAtWork hmm I will check out your branch tomorrow and see if I am getting the same error, stay tuned. |
@SebastianAtWork so I managed to reproduce the error in the |
vsintegration/src/FSharp.Editor/Refactor/RemoveExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
1065f22
to
d45ca33
Compare
vsintegration/tests/FSharp.Editor.Tests/Refactors/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
vsintegration/src/FSharp.Editor/Refactor/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
6118cd5
to
69f3e11
Compare
@psfinaki Sorry for the delay. I fixed your remarks and some refactorings. Should I add another Test case for when there are multiple documents in a project and one document infers a type from a previous document? Any other test cases I should add? |
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.
Good stuff. Left a few notes, definitely ready to review.
vsintegration/src/FSharp.Editor/Refactor/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
vsintegration/src/FSharp.Editor/Refactor/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/AddExplicitReturnTypeTests.fs
Outdated
Show resolved
Hide resolved
vsintegration/src/FSharp.Editor/Refactor/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
vsintegration/src/FSharp.Editor/Refactor/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/AddExplicitReturnTypeTests.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/RefactorTestFramework.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/RefactorTestFramework.fs
Outdated
Show resolved
Hide resolved
That would be helpful. At least to see how complicated is to have such a test. |
38a479a
to
6e83b22
Compare
vsintegration/tests/FSharp.Editor.Tests/Refactors/RefactorTestFramework.fs
Show resolved
Hide resolved
1814cc6
to
aa88e55
Compare
vsintegration/src/FSharp.Editor/Refactor/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
@SebastianAtWork awesome, so just then please add them, take your time and no rush! |
…peTests.fs - In AddExplicitReturnType.fs: - Replaced the codeActionFunc with a cancellableTask to handle cancellation tokens. - Updated the codeActionFunc to use the new cancellableTask and getChangedText functions. - In AddExplicitReturnTypeTests.fs: - Added tests for binding another function and binding linq function without crashing. - Updated the expectedCode for these tests to include explicit return types.
@psfinaki I added the 2 tests that pass and produce a compilable return type. I this correct or should it produce Unit->System.DateTime? |
@SebastianAtWork this is correct. We need return types, not the full signatures. Good job :) |
Will take a look at this tomorrow, but seems very promising - thanks for the updates! |
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.
Niiice! No major concerns anymore :)
I would just like to add a few more tests and remove the word explicit
. Otherwise good to go IMO.
vsintegration/tests/FSharp.Editor.Tests/Refactors/AddExplicitReturnTypeTests.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/AddExplicitReturnTypeTests.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/AddExplicitReturnTypeTests.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/AddExplicitReturnTypeTests.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/RefactorTestFramework.fs
Outdated
Show resolved
Hide resolved
vsintegration/tests/FSharp.Editor.Tests/Refactors/RefactorTestFramework.fs
Show resolved
Hide resolved
vsintegration/src/FSharp.Editor/Refactor/AddExplicitReturnType.fs
Outdated
Show resolved
Hide resolved
The code changes simplify the logic in the AddExplicitReturnType module. The changes remove unnecessary conditions and improve readability.
- Added a test case to ensure that the refactor correctly infers the return type of the `sum` function when it is defined on the next line. - Removed an unused import in the `RefactorTestFramework.fs` file. refactor: Add explicit return type to sum function
Just to understand, Why remove the explicit? Its also called AddExplicitTypeToParameter |
Yeah the refactoring is called like that - but the string is "Add type annotation" (as opposed to "add explicit type annotation") - so just to align with that one :) |
The commit updates the return type annotation in multiple localization files.
- Renamed the file `AddExplicitReturnType.fs` to `AddReturnType.fs`. - Updated references to the renamed file in other files. - Updated test file names and references accordingly.
@psfinaki Like this ? :) Done |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@dotnet/fsharp-team-msft please review, this is all green and resolved. |
Thanks for your contribution @SebastianAtWork. I am thinking now that once this gets to the VS preview, I might write a blog post about refactorings in VS where I could also mention this one and the new testing framework :) |
* upstream/main: (166 commits) typo in foldBack summary (dotnet#16453) Fix for dotnet#83 (improve constraint error message) (dotnet#16304) Name resolution: resolve interfaces in expressions (dotnet#15660) AddExplicitReturnType refactoring (dotnet#16077) Disabling 2 tests: running for too long, causing CI timeouts Improve value restriction error message dotnet#1103 (dotnet#15877) Parens: Keep parens for non-identical infix operator pairs with same precedence (dotnet#16372) More release note entries (dotnet#16438) Using Ordinal is both faster and more correct as our intent is to do … (dotnet#16439) merge (dotnet#16427) Optimize empty string compares (dotnet#16435) Checker: recover on unresolved type in 'inherit' member (dotnet#16429) Release notes proposal (dotnet#16377) [main] Update dependencies from dotnet/source-build-reference-packages (dotnet#16411) Allow usage of [<TailCall>] with older FSharp.Core package versions (dotnet#16373) Parser: recover on unfinished 'as' patterns (dotnet#16404) Parens: Keep parens in method calls in dot-lambdas (dotnet#16395) Checker: check unfinished obj expression inside computations (dotnet#16413) Added default dotnet-tools + additional tasks to launch them (dotnet#16409) make `remarks` and `returns` visible in quick info (dotnet#16417) ...
Thanks for helping :) Good Idea yes. On which blog? |
On Visual Studio .NET blog, here :) |
…and resource file entry
See Issue #15562 for Discussion
@psfinaki