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.

Sign up for GitHub

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

Jump to bottom

support fsautocomplete dll and remove obsolete code #4006

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

support fsautocomplete dll and remove obsolete code #4006

wants to merge 2 commits into from

Conversation

augustfengd
Copy link
Contributor

ionide has a similar feature, and emacs wants in on the fun. :)

@augustfengd
remove obsolete variable in fsharp client
f8fccaa 
this variable does not seem used since we install fsautocomplete in dotnet's
tool directory: 2d98a95.
@github-actions github-actions bot added the client One or more of lsp-mode language clients label Mar 26, 2023
elken
elken requested changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@elken elken left a comment

Choose a reason for hiding this comment

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

I don't understand why it's redundant, you're attempting to remove a variable that's used without even a deprecation notice.

@augustfengd
Copy link
Contributor Author

augustfengd commented Apr 15, 2023 •
edited

Hey @elken, I'm saying it's obsolete because the lsp-fsharp-server-install-dir looks like it was a location to install fsautocomplete and locate it it afterwards using an old installation method that's been removed (see the 2d98a95 that I linked in one of the commit messages). This commit is a bit more than a year old, so I'm thinking it's safe to remove this remaining "legacy" code now.

At worst, I imagine users will just be prompted to reinstall the language server using the new method in the backend.

@elken elken requested a review from razzmatazz April 15, 2023 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elken elken elken requested changes

@razzmatazz razzmatazz Awaiting requested review from razzmatazz

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
client One or more of lsp-mode language clients
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@augustfengd @elken