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

Fix installing fsautocomplete process #323

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

syohex
Copy link
Contributor

@syohex syohex commented Nov 8, 2022

  • Use cache for avoiding getting latest version
  • Fix version checking
  • Don't always install fsautocomplete

- Use cache for avoiding getting latest version
- Fix version checking
- Don't always install fsautocomplete
Comment on lines +67 to +71
(or eglot-fsharp--latest-version
(let* ((json (with-temp-buffer (url-insert-file-contents "https://azuresearch-usnc.nuget.org/query?q=fsautocomplete&prerelease=false&packageType=DotnetTool")
(json-parse-buffer)))
(versions (gethash "versions" (aref (gethash "data" json) 0))))
(setq eglot-fsharp--latest-version (gethash "version" (aref versions (1- (length versions))))))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use cache.

(defvar eglot-fsharp--latest-version nil "Latest fsautocomplete.exe version string.")

The cache variable is declared but it is never used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The purpose of (eglot-fsharp--latest-version) is to ALWAYS return the exact version string, so we can compare the version string of the locally installed version string and prevent downloading the nuget artefact.

Maybe I should document this better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@juergenhoetzel Should I remove cache variable ? Comparison is already implemented. Sorry I didn't catch what you mean.

(defun eglot-fsharp-current-version-p (version)
"Return t if the installation is not outdated."
(when (file-exists-p (eglot-fsharp--path-to-server))
(if (eq version 'latest)
(equal (eglot-fsharp--latest-version)
(eglot-fsharp--installed-version))
(equal eglot-fsharp-server-version (eglot-fsharp--installed-version)))))

Comment on lines -80 to +81
(if (eq version 'latest)
(if (eq eglot-fsharp-server-version 'latest)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

version argument is string type, so it is never 'latest.

(equal (eglot-fsharp--latest-version)
(eglot-fsharp--installed-version))
(equal eglot-fsharp-server-version (eglot-fsharp--installed-version)))))
(equal version (eglot-fsharp--installed-version)))))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

eglot-fsharp-server-version may be symbol. So version should be used here.

Comment on lines -100 to -104
(unless (zerop (call-process "dotnet" nil `(nil ,stderr-file) nil
"tool" "install" "fsautocomplete"
"--tool-path" default-directory "--version"
version))
(error "'dotnet tool install fsautocomplete --tool-path %s --version %s' failed" default-directory version)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code block is always evaluated even if desired version is installed. I suppose if this expression should be evaluated if (eglot-fsharp-current-version-p version) is nil

@syohex syohex marked this pull request as ready for review November 8, 2022 03:31
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.

None yet

2 participants