Skip to content
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

Update value in signature file. #1161

Merged
merged 6 commits into from
Jan 14, 2024

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Sep 5, 2023

WHAT

🤖 Generated by Copilot at ffe2cca

This pull request adds a new code fix feature that updates the value declaration in a signature file to match the implementation file, and integrates it into the language server and the test suite. It also refactors and cleans up some existing test modules and utilities for code fixes, and adds a new module for a code fix that converts string concatenation to interpolated strings.

🤖 Generated by Copilot at ffe2cca

We're the crew of the FsAutoComplete ship
We sail the seas of code with skill and wit
We fix the strings and values as we go
And test them all with testCaseAsync so

🧵🔄🧪

WHY

HOW

🤖 Generated by Copilot at ffe2cca

  • The ToInterpolatedString.fix function converts string concatenation expressions to interpolated strings using the $ syntax (link)
  • The UpdateValueInSignatureFile.fix function updates the value declaration in a signature file to match the implementation file using the FSharpSymbolUse information (link)
  • The signature files ToInterpolatedString.fsi and UpdateValueInSignatureFile.fsi declare the public types and values of the new code fix modules (link, link)
  • The AdaptiveFSharpLspServer and FSharpLspServer types can handle the new code fixes by adding them to the list of code fix providers (link, link)
  • The UpdateValueInSignatureFileTests.fs file contains the test cases for the UpdateValueInSignatureFile code fix, using the checkFixAt function with different document identifiers (link)
  • The tests function in Tests.fs runs the new code fix tests along with the existing ones (link)
  • Enhance the test utilities to support code fixes that affect multiple files (link, link, link, link)
    • The checkFixAt function takes a new parameter editsFrom that specifies the text document identifier from which the code fix edits should be extracted (link)
    • The checkFixAt function applies the code fix edits to the document specified by editsFrom, which may not be the same as the document that triggered the code fix (link)
    • The checkFix and checkFixAll functions pass the document identifier of the untitled or shared document to the checkFixAt function, which is the same as the document that triggered the code fix (link, link)
  • Format and refactor the test code and utilities for readability and consistency (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
    • The RenameParamToMatchSignatureTests.fs file adds new lines before each testCaseAsync expression to improve the readability and consistency of the test code (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
    • The RenameParamToMatchSignatureTests.fs file removes an extra empty line at the end of the module (link)
    • The CursorbasedTests.fs and CursorbasedTests.fsi files remove some unused open statements from the Utils.CursorbasedTests module (link, link, link, link)
    • The CursorbasedTests.fs file removes some extra spaces and new lines from the diagnosticsIn, checkFixAt, and checkFixAll functions (link, link, link, link)

Dog feeding #1158 a bit.
I think I'm doing something wrong on the Expecto side. dotnet run isn't exiting correctly.

The actual code fix needs to be expanded a bit more. I would, however, very much like to have this.
I could use a first round of feedback if anybody is up for it.

@nojaf nojaf force-pushed the UpdateValueInSignatureFile branch 2 times, most recently from 3b98c88 to 53d17ad Compare November 9, 2023 16:04
@nojaf nojaf changed the base branch from main to nightly November 9, 2023 16:05
@nojaf nojaf changed the base branch from nightly to main December 20, 2023 09:29
@nojaf
Copy link
Contributor Author

nojaf commented Dec 20, 2023

This can actually be solved quite easily if the newly added extended data is available.

image

@baronfel any ideas on how we want to expose this optional information in the Diagnostic type?

@nojaf nojaf changed the title POC: update value in signature file. Update value in signature file. Dec 22, 2023
@nojaf nojaf marked this pull request as ready for review December 22, 2023 08:18
@nojaf
Copy link
Contributor Author

nojaf commented Dec 22, 2023

@TheAngryByrd I'm able to get the extended data from the diagnostic in the check results.
This is heaven-sent.

@baronfel
Copy link
Contributor

This is great @nojaf - using extended data was what came to my mind immediately. Let me take a look at the way you're using it - we may want to think of some kind of protocol for single-fix vs whole-file fixes, for example.

@TheAngryByrd
Copy link
Member

I know LSP kind of defines categories but they’re not explicit. Had to go looking thru the vscode code to see what was supported.

@nojaf
Copy link
Contributor Author

nojaf commented Dec 24, 2023

Let me take a look at the way you're using it - we may want to think of some kind of protocol for single-fix vs whole-file fixes, for example.

Could that be part of a separate effort? I feel like this one is good to go, nothing to lose here.

And could someone re-run the failed builds?
I don't think all of them are problematic.
Thx!

@TheAngryByrd
Copy link
Member

And could someone re-run the failed builds?
I don't think all of them are problematic.

3 of them (mac/ubuntu) seem to be formatting failures.

src/FsAutoComplete.Core/Commands.fs needs formatting

Is this just newline/encoding nonsense?

@nojaf
Copy link
Contributor Author

nojaf commented Dec 24, 2023

Serious doubt there. Did you re-run them?

@nojaf
Copy link
Contributor Author

nojaf commented Jan 14, 2024

Could this one get merged please?

Copy link
Member

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ thank you!

@TheAngryByrd TheAngryByrd merged commit 449e534 into ionide:main Jan 14, 2024
10 of 14 checks passed
@nojaf
Copy link
Contributor Author

nojaf commented Jan 14, 2024

Thanks Jimmy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants