-
Notifications
You must be signed in to change notification settings - Fork 44
Add setting to use bundled Quarto in Positron #841
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
Conversation
I don't think we can add a meaningful test for this because we don't have Positron test runner builds, and we don't have more than one version of Quarto installed in the CLI currently. Those could be areas for future test investment, but probably not the highest priority for testing currently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this (and "what Quarto am I?" is a real source of trouble in our issues, so all help is welcome here). With that said:
- I'm not the right person to do a code review on the positron- and vs-code-specific paths of the code.
- Maybe this is for a separate PR, but it would be good for someone (Positron? the extension?) to surface the result of that quarto-finding logic in a legible way for users, even if only in logs. For example: in the CLI, we have
quarto check
, which produces a report of which versions of dependencies are being used (and where they are in the file system), etc. This way, when something goes wrong, we have a standard flow: "show usquarto check
, and we'll go from there"
That's a great point; thanks! I have logged how to surface the Quarto CLI discovery better in #846. @isabelizimm can I get you to review this PR, especially with an eye to how we handle this new setting relative to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this make more sense as a dropdown setting of "Use alternate Quarto" with "off", "pipQuarto" and "builtinQuarto (Positron only)" options? It would help clarify the precedence IMO, but might add in different confusion with IDE-dependent support. Don't feel strongly about this, but may be helpful?
Something like files.autoSave
, where you can add the extra info at the bottom as you browse options:
event.affectsConfiguration(setting)).join(", ")}`); | ||
|
||
// Prompt user to restart | ||
vscode.window.showInformationMessage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to do a little bit of checking here for precedence, this notification will show up even in scenarios where the path settings have not actually changed, like if you turn on bundled quarto in VSCode.
Thank you, @isabelizimm! 🙌 I actually don't think it would be great to mix Positron-specific and general settings in the same dropdown; I think I'd prefer to keep as is. |
Took a look over and it makes sense to me! I added an idea for how to surface the Quarto source to users in #846. |
Addresses #719
This PR adds a new setting to prefer Positron's bundled Quarto. The setting defaults to false, and I put it between
quarto.path
andquarto.usePipQuarto
in terms of precedence. It could easily go belowquarto.usePipQuarto
, if we think that's better.This PR also introduces the use of the
@posit-dev/positron
npm package, as described here:https://positron.posit.co/extension-development.html
There are a LOT of improvements we could make to the Positron-specific code throughout the extension, but let's do that gradually.
This PR also introduces listening for config changes for all three PATH config options, to prompt the user to reload the window. These settings need a restart to work correctly, or at least for the changes to be reflected in the status bar? But IIUC you really do need a restart.
How to test this out
.vsix
or make a dev build)quarto.useBundledQuartoInPositron
, and enable it