Rename param to match signature code fix #917
Conversation
Fix capitalization
Rename parameter name to match its signature (in fsi file) ("The argument names in the signature 'xxx' and implementation 'yyy' do not match.")
Note: sig name is extracted from diag message. But this fails when:
* diag message is english and sig name contains `' and implementation '`
* diag message isn't english and sig or impl name contains `'` (excluding last position)
I think that's acceptable:
Sig name is public -> unlikely to contain exact phrase with `'` and spaces
Port of `dotnet/fsharp` (VS) -> [`RenameParamToMatchSignature`](https://github.com/dotnet/fsharp/blob/main/vsintegration/src/FSharp.Editor/CodeFix/RenameParamToMatchSignature.fs)
Add tests for `RenameParamToMatchSignature`
Note: Tests are a bit more complicated than other CodeFix tests: Don't work with untitled script files, but instead require a `fsi` and corresponding `fs` file.
-> Slight hack to be able to specify source in each test: Open existing `fsi` and `fs` file, but with text specified in each test.
Add `itestCaseAsync`: `ignore testCaseAsync`:
Like `testCaseAsync`, but test gets completely ignored.
Unlike `ptestCaseAsync` (pending), `itestCaseAsync` doesn't even show up in Expecto summary.
-> Used to mark issues & shortcomings in CodeFixes, but without any (immediate) intention to fix (vs. `pending` -> marked for fixing)
-> ~ uncommenting tests without actual uncommenting
* Example: used to ignore test with sig `' and implementation '`
-> shortcoming gets highlighted, but unlike `pending` it's considered "ok for now"
Uncomment unused tests with `itestCaseAsync` instead of `//`
| let private renameParamToMatchSignatureTests state = | ||
| let selectCodeFix expectedName = CodeFix.withTitle (RenameParamToMatchSignature.title expectedName) | ||
|
|
||
| // requires `fsi` and corresponding `fs` file (and a project!) |
There was a problem hiding this comment.
you could potentially generate this at test-time to a random directory instead of being forced to check in empty data?
There was a problem hiding this comment.
The "complex" part here is the project file.
Which of course can be written to file too -- but now it's either content of fsproj in tests or adjusting a freshly created project (with dotnet new ... in some dir).
And would require getting a dir, and then writing all necessary files....but that adds additional complexity
We're now way more complex than just loading an existing project -- even when just project just contains empty "dummy" files
There was a problem hiding this comment.
Not a need for now, but I've been looking at this library for defining project files in code
|
Forcing a specific localization can be done via environment variable, but we should definitely take care to force the InvariantCulture at Program.fs for the apps and the tests (though This is a good fix :) Regarding some of the questions you bring up - is this a problem with the error code not being unique enough to only use the code as a determining factor? |
The diag is unique. BUT: the error message contains the signature name:
-> CodeFix (here and in VS) extracts name from diag message I just added the additional match for exact english match because there are issues when param name contains |
Fixed in #926 without anything from this PR -> PR is done (though 2 Runs failed? usually just windows is out of line 😞 ) |
|
Generally macos/ubuntu failures are transient, and the windows failure is transient with one specific test (that I think is due to running multiple tests from the same data directory). It's something to look at, but haven't found the time yet. Usually a poke fixes it up, like it did here. |
|
Again, thank you for one more wonderfully detailed and thoroughly-tested PR! |
Code Fix to rename parameter in implementation (in
fsfile) to match its signature (infsifile)Port of
dotnet/fsharp(VS) ->RenameParamToMatchSignature(Note: if you try the example above yourself, you'll see an additional
Add explicit type annotationCodeFix -- which is incorrect. Fix will come soon (Uses Test helper introduced in this PR))But there are situations this CodeFix will not produce the correct result (suggests wrong name):
' and implementation ''(except last position)'s with Regex -> incorrect match when param name contains additional'(-> algo for getting sig param name: First try exact match in english, then fall back to regex between
')I think that's acceptable:
Extra Signature is extremely unlike to contain
' and implementation '.Fallback (not english) is more likely to fail. But even one
'(especially not at end of name) in signature name is quite unlikely too. Might occur in impl name, but probably not inside param name (just at end).-> I think it's good enough and not worth doing something more complicated
Note:
Matching based on localization is an issue for tests: Multiple tests will fail when FCS returns diag messages in something other than english.
(Does FCS even auto-localize? Compilers and their tools that use local language by default are just horrible. Even just googling an error message becomes complicated....)
Edit:
Missing CodeFixes from VS:
Fixin FSAC only handles single doc (same for tests) -> requires changes(-> not now -- maybe sometime later)
arr[2]->arr.[2]. Isn't an error any more -- even recommended now -> would only apply to old F# compiler versions-> I don't think it's worth implementing this CodeFix
(other direction might be worth it: remove
.)