Skip to content

Add option for setting manual path to Fourmolu binary #3860

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

Merged
merged 6 commits into from
Jan 16, 2024

Conversation

georgefst
Copy link
Collaborator

@georgefst georgefst commented Nov 5, 2023

I should probably look in to making this work more like serverExecutablePath, where it's empty by default, and allows expands ~. But I'm not sure where that logic lives - I see no results in the whole codebase when searching for "serverExecutablePath".

@fendor
Copy link
Collaborator

fendor commented Nov 6, 2023

serverExecutablePath is a feature implemented by vscode-haskell, not HLS! And the support for ~ is a simple search and replace operation.
Here is where we resolve serverExecutablePath: https://github.com/haskell/vscode-haskell/blob/master/src/hlsBinaries.ts#L136
Examples for resolving ~ and ${HOME}: https://github.com/haskell/vscode-haskell/blob/master/src/utils.ts#L181 and https://github.com/haskell/vscode-haskell/blob/master/src/utils.ts#L188

However, the idea is the same, if the config option is either "" or null, then use the built-in formatter, otherwise use the PATH.

@michaelpj
Copy link
Collaborator

Can we ask the user's shell to expand the path? That's the limiting case of what we want, right?

@michaelpj
Copy link
Collaborator

Returning to this: I think it's pretty sensible for HLS to expect an actual full path, and for it to be the job of the client to shell-expand paths or whatever. So I think the current implementation of this seems fine.

Should we update and merge this?

@georgefst
Copy link
Collaborator Author

Yeah, sure. If anyone really wants to add expansion then there's nothing lost by that being a second PR.

@georgefst georgefst marked this pull request as ready for review January 10, 2024 10:36
@soulomoon
Copy link
Collaborator

LGTM, maybe we can update merge this.

@georgefst georgefst requested a review from fendor as a code owner January 16, 2024 11:24
@fendor fendor added the status: needs review This PR is ready for review label Jan 16, 2024
@soulomoon soulomoon self-requested a review January 16, 2024 11:44
@fendor fendor enabled auto-merge January 16, 2024 12:56
@fendor fendor added merge me Label to trigger pull request merge and removed status: needs review This PR is ready for review labels Jan 16, 2024
@fendor fendor merged commit 4361687 into master Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants