search

fortran-lang / fortls

fortls - Fortran Language Server
https://fortls.fortran-lang.org/
MIT License
258 stars 41 forks source link

Allow conda autoupdate #346

Closed emanspeaks closed 10 months ago

emanspeaks commented 10 months ago

Disabled by default, but provides the option to allow autoupdating fortls when installed in a conda environment for those of us who do desire that functionality and don't mind it changing the environment.

Additionally fixes bug in unit test for updater where config file was not being properly loaded by the LangServer instance.

codecov[bot] commented 10 months ago

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (8263781) 87.51% compared to head (4182503) 87.36%.

Files Patch % Lines
fortls/langserver.py 60.00% 10 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #346 +/- ## ========================================== - Coverage 87.51% 87.36% -0.15% ========================================== Files 35 35 Lines 4756 4771 +15 ========================================== + Hits 4162 4168 +6 - Misses 594 603 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

emanspeaks commented 10 months ago

You do provide a conda-forge feedstock for this repo, though. That implies it should be a supported way to install and update the package. I am not advocating for supporting all environment managers (e.g. poetry) because you aren't offering those. If not here, though, would the vscode extension be a better place to implement this? I feel like that should be opinionated about how it is installed even if the language server itself is not. I don't want my teammates to just never update their fortls because they will ignore the messages. I think it would be a convenient feature for others to have as well.

gnikit commented 10 months ago

We offer multiple ways of installing, at no point does that imply that we should add code to handle autoupdates for each package manager and distribution channel. This is simply not sustainable nor a good way of separating responsibilities between the application and the various installers.

The conda exclusion was added explicitly because Anaconda users didn't want to have Python module self-updating. For homebrew such functionality can be considered a disqualifying feature for a package see. If one is using a proper package manager like conda or brew it is ultimately their responsibility to keep it updated and the tool's responsibility not to break their environment.

emanspeaks commented 10 months ago

If not here, though, would the vscode extension be a better place to implement this?

Since this change is not desired here, would it be better entertained in the vscode-fortran-support extension? In hindsight, that seems a better place for something like this in any case; I pushed the PR here primarily since the PyPI feature was here. I can try to recreate that capability there instead if you would be supportive of allowing that feature there.

There is still a bug in the server interface unit tests captured here I'd like to merge, so I can create a new PR targeted for that fix and, pending resolution of this conversation, this one could then be closed.

Just let me know what you think about me moving this feature to the vscode extension so I don't spin my wheels on it over there too!

emanspeaks commented 10 months ago

Closing this in lieu of #347 and https://github.com/fortran-lang/vscode-fortran-support/issues/1037