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 form for customizing superbol.path #72

Merged
merged 4 commits into from
Oct 30, 2023
Merged

Conversation

Stevendeo
Copy link
Contributor

This PR fixes the superbol.path form that was not displayed because of a type error.

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2023

CLA assistant check
All committers have signed the CLA.

@Stevendeo Stevendeo requested a review from lefessan October 26, 2023 10:47
@nberth
Copy link
Collaborator

nberth commented Oct 26, 2023

This form seemed to show correctly to me before. Was there a specific issue with its rendering?

While we are at it, is there a way to be more specific, and, for instance, tell it's a(n existing) directory that we'd want in this field?

@Stevendeo
Copy link
Contributor Author

Stevendeo commented Oct 26, 2023

This form seemed to show correctly to me before. Was there a specific issue with its rendering?

On my side, it displays a link to the settings.json file to edit the option manually. Which version of vscode do you use? I am using 1.82.2.

While we are at it, is there a way to be more specific, and, for instance, tell it's a(n existing) directory that we'd want in this field?

I'm trying to make variable substitution work, the doc says something like ${env:PATH} should work, but it does not (at least on my version).

@nberth
Copy link
Collaborator

nberth commented Oct 26, 2023

On my side, it displays a link to the settings.json file to edit the option manually. Which version of vscode do you use? I am using 1.82.2.

Hum not good news: I'm using 1.80.0. So I assume the current specs are a just becoming out-dated.

@Stevendeo
Copy link
Contributor Author

Stevendeo commented Oct 26, 2023

Variable substitution does not seem to work either on the last version (1.83.1), it would have been too easy 😅

@Stevendeo
Copy link
Contributor Author

Stevendeo commented Oct 26, 2023

Good thing, though: putting superbol alone in the field seems to be sufficient.

EDIT : #75 adresses the issue

@Stevendeo Stevendeo mentioned this pull request Oct 26, 2023
Copy link
Contributor

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

Always good to follow the specs for package.json :-)
Please improve the one description and the types for the others.

@@ -21,6 +21,7 @@
"description": "If something is selected, only format the selection"
},
"superbol.path": {
"type": "string",
"default": "",
"description": "Path to the `superbol` command"
Copy link
Contributor

Choose a reason for hiding this comment

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

please adjust that, maybe something like "name of superbol command/binary to use, may be full path", at least that would tell me that I can either use something that's on the environment specific PATH or a full path.

package.json Outdated Show resolved Hide resolved
@Stevendeo Stevendeo mentioned this pull request Oct 26, 2023
~description: "Path to the `superbol` command"
~description:
"Name of the `superbol` command; path can be relative (deprecated) or \
absolute."
Copy link
Collaborator

@nberth nberth Oct 27, 2023

Choose a reason for hiding this comment

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

This does not seem to convey the idea that one can put an executable name that will be searched in PATH. Is it actually allowed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is allowed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then I'd suggest something like "Name of the superbol executable if available in PATH; otherwise, may be an absolute path."

@GitMensch
Copy link
Contributor

@nberth If you want changes to this PR then please review requesting changes. Otherwise I suggest to pull this in to get the type error fixed (we can easily adjust the description later).

nberth
nberth previously approved these changes Oct 30, 2023
Copy link
Collaborator

@nberth nberth left a comment

Choose a reason for hiding this comment

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

One small suggestion and we can pull this in ;-)

~description: "Path to the `superbol` command"
~description:
"Name of the `superbol` command; path can be relative (deprecated) or \
absolute."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then I'd suggest something like "Name of the superbol executable if available in PATH; otherwise, may be an absolute path."

@nberth nberth self-requested a review October 30, 2023 08:09
Copy link
Collaborator

@nberth nberth left a comment

Choose a reason for hiding this comment

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

Wrong button on previous review: actually requesting a change.

@Stevendeo Stevendeo requested a review from nberth October 30, 2023 09:48
Copy link
Collaborator

@nberth nberth left a comment

Choose a reason for hiding this comment

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

Ok to merge after fixing typo ;-)

src/lsp/superbol_free_lib/project.ml Outdated Show resolved Hide resolved
@nberth nberth changed the title Parameter window display form to select path Fix form for customizing superbol.path Oct 30, 2023
@nberth nberth merged commit cfc35f1 into OCamlPro:master Oct 30, 2023
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.

4 participants