Support variable substitution in packagePath and packageCachePath#2406
Support variable substitution in packagePath and packageCachePath#2406mohe2015 wants to merge 3 commits into
packagePath and packageCachePath#2406Conversation
mohe2015
requested review from
Enter-tainer and
Myriad-Dreamin
as code owners
Summary of ChangesHello @mohe2015, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the configuration flexibility of the Typst language server by introducing dedicated settings for custom package and package cache paths. This change allows users to override default system-dependent locations, providing more control over their Typst development environment and addressing previously reported issues related to package management. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for variable substitution in packagePath and packageCachePath, and also adds a new convertExtension configuration. The changes are well-implemented. I have provided a few suggestions to enhance code readability in one of the Rust files and to address minor inconsistencies and a formatting error in the documentation files.
|
I kept the changes that are produced by the doc regeneration but are not related to my changes. They probably should be done directly on main instead. |
|
@gemini-code-assist review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for variable substitution in packagePath and packageCachePath configurations. The changes correctly update the Rust configuration handling, the VS Code extension's variable substitution logic, and related documentation files. My review includes a suggestion to simplify the code in config.rs for better readability, fixes for minor typos in the documentation, and a note about missing translations to ensure localization is complete. Overall, the implementation is solid and addresses the intended feature.
| let package_path = self.package.package_path.as_ref() | ||
| .or_else(|| extra_package_opts.and_then(|p| p.package_path.as_ref())) | ||
| .cloned(); | ||
|
|
||
| let package_cache_path = self.package.package_cache_path.as_ref() | ||
| .or_else(|| extra_package_opts.and_then(|p| p.package_cache_path.as_ref())) | ||
| .cloned(); |
There was a problem hiding this comment.
For better readability and to use a more direct method, you can replace or_else with or here. Since the expression extra_package_opts.and_then(|p| p.package_path.as_ref()) doesn't involve a closure and returns an Option, or is more idiomatic.
| let package_path = self.package.package_path.as_ref() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_path.as_ref())) | |
| .cloned(); | |
| let package_cache_path = self.package.package_cache_path.as_ref() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_cache_path.as_ref())) | |
| .cloned(); | |
| let package_path = self.package.package_path.as_ref() | |
| .or(extra_package_opts.and_then(|p| p.package_path.as_ref())) | |
| .cloned(); | |
| let package_cache_path = self.package.package_cache_path.as_ref() | |
| .or(extra_package_opts.and_then(|p| p.package_cache_path.as_ref())) | |
| .cloned(); |
|
|
||
| This configuration item can be one of following types: | ||
|
|
||
| - - **Type**: `array` |
There was a problem hiding this comment.
There seems to be a typo in the markdown list format. The line starts with - - which is likely unintentional. It should be a single hyphen for a list item.
| - - **Type**: `array` | |
| - **Type**: `array` |
|
|
||
| This configuration item can be one of following types: | ||
|
|
||
| - - **Type**: `array` |
There was a problem hiding this comment.
There appears to be a typo in the markdown list format. The line starts with - -, which should probably be a single hyphen - for a standard list item.
| - - **Type**: `array` | |
| - **Type**: `array` |
| [extension.tinymist.config.tinymist.packagePath.title] | ||
| en = "Custom path to local packages" | ||
|
|
||
| [extension.tinymist.config.tinymist.packagePath.desc] | ||
| en = "Custom path to local packages, defaults to system-dependent location" | ||
|
|
||
| [extension.tinymist.config.tinymist.packageCachePath.title] | ||
| en = "Custom path to package cache" | ||
|
|
||
| [extension.tinymist.config.tinymist.packageCachePath.desc] | ||
| en = "Custom path to package cache, defaults to system-dependent location" | ||
|
|
There was a problem hiding this comment.
The newly added configuration options are missing their Chinese (zh) translations. Please add them to maintain localization consistency across the project.
There was a problem hiding this comment.
Code Review
This pull request adds support for variable substitution in packagePath and packageCachePath configurations. It introduces these settings at the top level, allowing them to be configured directly, and updates the logic to correctly prioritize them over settings from typstExtraArgs. The changes also include necessary updates to documentation, VS Code extension manifest, and localization files. Additionally, a documentation fix for convertExtension and an updated link for mason.nvim are included.
My review identifies a few areas for improvement:
- Refactoring a piece of Rust code for better readability and idiomatic style.
- Correcting a typo in the markdown documentation.
- Ensuring alphabetical ordering of configuration keys in
package.jsonandlocales/tinymist-vscode.tomlfor consistency.
| let package_path = self.package.package_path.as_ref() | ||
| .or_else(|| extra_package_opts.and_then(|p| p.package_path.as_ref())) | ||
| .cloned(); | ||
|
|
||
| let package_cache_path = self.package.package_cache_path.as_ref() | ||
| .or_else(|| extra_package_opts.and_then(|p| p.package_cache_path.as_ref())) | ||
| .cloned(); |
There was a problem hiding this comment.
This logic can be written more idiomatically using clone() and or_else to avoid as_ref() and .cloned().
| let package_path = self.package.package_path.as_ref() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_path.as_ref())) | |
| .cloned(); | |
| let package_cache_path = self.package.package_cache_path.as_ref() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_cache_path.as_ref())) | |
| .cloned(); | |
| let package_path = self.package.package_path.clone() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_path.clone())); | |
| let package_cache_path = self.package.package_cache_path.clone() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_cache_path.clone())); |
|
|
||
| This configuration item can be one of following types: | ||
|
|
||
| - - **Type**: `array` |
There was a problem hiding this comment.
There seems to be a typo here with the double dash. It should probably be a single dash for the list item.
| - - **Type**: `array` | |
| - **Type**: `array` |
|
|
||
| This configuration item can be one of following types: | ||
|
|
||
| - - **Type**: `array` |
There was a problem hiding this comment.
There seems to be a typo here with the double dash. It should probably be a single dash for the list item.
| - - **Type**: `array` | |
| - **Type**: `array` |
| "tinymist.packagePath": { | ||
| "title": "%extension.tinymist.config.tinymist.packagePath.title%", | ||
| "markdownDescription": "%extension.tinymist.config.tinymist.packagePath.desc%", | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ], | ||
| "default": null | ||
| }, | ||
| "tinymist.packageCachePath": { | ||
| "title": "%extension.tinymist.config.tinymist.packageCachePath.title%", | ||
| "markdownDescription": "%extension.tinymist.config.tinymist.packageCachePath.desc%", | ||
| "type": [ | ||
| "string", | ||
| "null" | ||
| ], | ||
| "default": null | ||
| }, |
There was a problem hiding this comment.
For consistency, it would be better to keep the configuration properties sorted alphabetically. tinymist.packageCachePath should come before tinymist.packagePath.
| "tinymist.packagePath": { | |
| "title": "%extension.tinymist.config.tinymist.packagePath.title%", | |
| "markdownDescription": "%extension.tinymist.config.tinymist.packagePath.desc%", | |
| "type": [ | |
| "string", | |
| "null" | |
| ], | |
| "default": null | |
| }, | |
| "tinymist.packageCachePath": { | |
| "title": "%extension.tinymist.config.tinymist.packageCachePath.title%", | |
| "markdownDescription": "%extension.tinymist.config.tinymist.packageCachePath.desc%", | |
| "type": [ | |
| "string", | |
| "null" | |
| ], | |
| "default": null | |
| }, | |
| "tinymist.packageCachePath": { | |
| "title": "%extension.tinymist.config.tinymist.packageCachePath.title%", | |
| "markdownDescription": "%extension.tinymist.config.tinymist.packageCachePath.desc%", | |
| "type": [ | |
| "string", | |
| "null" | |
| ], | |
| "default": null | |
| }, | |
| "tinymist.packagePath": { | |
| "title": "%extension.tinymist.config.tinymist.packagePath.title%", | |
| "markdownDescription": "%extension.tinymist.config.tinymist.packagePath.desc%", | |
| "type": [ | |
| "string", | |
| "null" | |
| ], | |
| "default": null | |
| }, |
| [extension.tinymist.config.tinymist.packagePath.title] | ||
| en = "Custom path to local packages" | ||
|
|
||
| [extension.tinymist.config.tinymist.packagePath.desc] | ||
| en = "Custom path to local packages, defaults to system-dependent location" | ||
|
|
||
| [extension.tinymist.config.tinymist.packageCachePath.title] | ||
| en = "Custom path to package cache" | ||
|
|
||
| [extension.tinymist.config.tinymist.packageCachePath.desc] | ||
| en = "Custom path to package cache, defaults to system-dependent location" | ||
|
|
There was a problem hiding this comment.
For consistency with other configuration files and general good practice, it would be better to keep these new configuration keys sorted alphabetically. packageCachePath should come before packagePath.
| [extension.tinymist.config.tinymist.packagePath.title] | |
| en = "Custom path to local packages" | |
| [extension.tinymist.config.tinymist.packagePath.desc] | |
| en = "Custom path to local packages, defaults to system-dependent location" | |
| [extension.tinymist.config.tinymist.packageCachePath.title] | |
| en = "Custom path to package cache" | |
| [extension.tinymist.config.tinymist.packageCachePath.desc] | |
| en = "Custom path to package cache, defaults to system-dependent location" | |
| [extension.tinymist.config.tinymist.packageCachePath.title] | |
| en = "Custom path to package cache" | |
| [extension.tinymist.config.tinymist.packageCachePath.desc] | |
| en = "Custom path to package cache, defaults to system-dependent location" | |
| [extension.tinymist.config.tinymist.packagePath.title] | |
| en = "Custom path to local packages" | |
| [extension.tinymist.config.tinymist.packagePath.desc] | |
| en = "Custom path to local packages, defaults to system-dependent location" | |
|
I will wait for a human. This does not look productive to me. |
There was a problem hiding this comment.
Code Review
This pull request adds support for variable substitution in packagePath and packageCachePath configurations, and also introduces a new convertExtension configuration. The changes span across the core logic, VSCode extension configuration, and documentation. The implementation looks good overall. I've found a couple of minor typos in the documentation and a missing translation in the localization file.
| pub fn package_opts(&self) -> CompilePackageArgs { | ||
| if let Some(extras) = &self.typst_extra_args { | ||
| return extras.package.clone(); | ||
| let extra_package_opts = self.typst_extra_args.as_ref().map(|a| &a.package); | ||
|
|
||
| let package_path = self.package.package_path.as_ref() | ||
| .or_else(|| extra_package_opts.and_then(|p| p.package_path.as_ref())) | ||
| .cloned(); | ||
|
|
||
| let package_cache_path = self.package.package_cache_path.as_ref() | ||
| .or_else(|| extra_package_opts.and_then(|p| p.package_cache_path.as_ref())) | ||
| .cloned(); | ||
|
|
||
| CompilePackageArgs { | ||
| package_path, | ||
| package_cache_path, | ||
| } | ||
| CompilePackageArgs::default() | ||
| } |
There was a problem hiding this comment.
This implementation can be simplified to be more concise and readable by avoiding intermediate variables and directly constructing the CompilePackageArgs struct.
| pub fn package_opts(&self) -> CompilePackageArgs { | |
| if let Some(extras) = &self.typst_extra_args { | |
| return extras.package.clone(); | |
| let extra_package_opts = self.typst_extra_args.as_ref().map(|a| &a.package); | |
| let package_path = self.package.package_path.as_ref() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_path.as_ref())) | |
| .cloned(); | |
| let package_cache_path = self.package.package_cache_path.as_ref() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_cache_path.as_ref())) | |
| .cloned(); | |
| CompilePackageArgs { | |
| package_path, | |
| package_cache_path, | |
| } | |
| CompilePackageArgs::default() | |
| } | |
| pub fn package_opts(&self) -> CompilePackageArgs { | |
| let extra_package_opts = self.typst_extra_args.as_ref().map(|a| &a.package); | |
| CompilePackageArgs { | |
| package_path: self | |
| .package | |
| .package_path | |
| .as_ref() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_path.as_ref())) | |
| .cloned(), | |
| package_cache_path: self | |
| .package | |
| .package_cache_path | |
| .as_ref() | |
| .or_else(|| extra_package_opts.and_then(|p| p.package_cache_path.as_ref())) | |
| .cloned(), | |
| } | |
| } |
|
|
||
| This configuration item can be one of following types: | ||
|
|
||
| - - **Type**: `array` |
There was a problem hiding this comment.
There appears to be a typo in the list formatting. It should be a single hyphen.
| - - **Type**: `array` | |
| - **Type**: `array` |
|
|
||
| This configuration item can be one of following types: | ||
|
|
||
| - - **Type**: `array` |
There was a problem hiding this comment.
There appears to be a typo in the list formatting. It should be a single hyphen.
| - - **Type**: `array` | |
| - **Type**: `array` |
| [extension.tinymist.config.tinymist.packagePath.title] | ||
| en = "Custom path to local packages" | ||
|
|
||
| [extension.tinymist.config.tinymist.packagePath.desc] | ||
| en = "Custom path to local packages, defaults to system-dependent location" | ||
|
|
||
| [extension.tinymist.config.tinymist.packageCachePath.title] | ||
| en = "Custom path to package cache" | ||
|
|
||
| [extension.tinymist.config.tinymist.packageCachePath.desc] | ||
| en = "Custom path to package cache, defaults to system-dependent location" | ||
|
|
There was a problem hiding this comment.
The Chinese (zh) translations for the new configuration options (packagePath and packageCachePath) are missing. Please add them to maintain localization consistency.
1 participant
Close #2304
Close #2403