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

Codefix to add private access modifier #1089

Merged
merged 15 commits into from
Apr 1, 2023
Merged

Conversation

dawedawe
Copy link
Contributor

@dawedawe dawedawe commented Mar 25, 2023

WHAT

🤖 Generated by Copilot at 2a2c771

This pull request refactors some functions in the adaptive F# LSP server, implements a code fix for adding a private access modifier in both LSP servers, and adds a new module for the code fix logic. The code fix helps users avoid exposing unnecessary symbols and follows the F# style guide. The refactoring improves the code structure and avoids circular dependencies.

🤖 Generated by Copilot at 2a2c771

We are the masters of the code, we fix the errors and the flaws
We use the syntax visitor, we add the private modifier
We refactor and we improve, we move the functions and we groove
We support the LSP server, we are the symbolUseWorkspace lovers

🛠️♻️🚀

WHY

This is the somewhat escalated result of yesterdays Amplify F# session.
As Jimmy pointed out, it might be too expensive to have something like this enabled by default.
But if there is interest in having this behind a config option or maybe in a trimmed down version, I'd be happy to adjust it.

HOW

🤖 Generated by Copilot at 2a2c771

  • Add a new module AddPrivateAccessModifier that implements a code fix for adding a private access modifier to a declaration that is not used outside of its scope (link)
  • Move the getDependentProjectsOfProjects function to the top of the file AdaptiveFSharpLspServer.fs to avoid a circular dependency between the getDeclarationLocation and the symbolUseWorkspace functions (link)
  • Move the getDeclarationLocation and the symbolUseWorkspace functions to the top of the file AdaptiveFSharpLspServer.fs to avoid a circular dependency between them (link)
  • Add a new function symbolUseWorkspace to the type AdaptiveFSharpLspServer that wraps the Commands.symbolUseWorkspace function with the specific arguments and functions needed for the adaptive server (link)
  • Add a new function symbolUseWorkspace to the type FSharpLspServer that wraps the commands.SymbolUseWorkspace function with the specific arguments and functions needed for the non-adaptive server (link)
  • Add the code fix for adding a private access modifier to the list of code fixes supported by the type FSharpLspServer (link)

@baronfel
Copy link
Contributor

If it's expensive I would appreciate a config flag like the other 'enabled' flags. Happy to merge this when that's in.

@TheAngryByrd
Copy link
Member

If it's expensive I would appreciate a config flag like the other 'enabled' flags

Yeah it's using Find All References here to determine if it provide the check. While #1088 does make this super quick for smaller repositories, it can still be 5-10 seconds for big ones like FSharp.Core. Still worth a flag for it I think.

@dawedawe
Copy link
Contributor Author

Great, I'll add a flag then.

@dawedawe
Copy link
Contributor Author

Okay, the flag is added and the counterpart in Ionide is here: ionide/ionide-vscode-fsharp#1858

@baronfel
Copy link
Contributor

baronfel commented Apr 1, 2023

Thanks for this @dawedawe!

@baronfel baronfel merged commit 3b1a8cb into ionide:main Apr 1, 2023
@dawedawe dawedawe deleted the amplifysession branch April 1, 2023 18:50
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