-
Notifications
You must be signed in to change notification settings - Fork 30
Add configuration options for update checks and custom component paths #132
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
base: main
Are you sure you want to change the base?
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @himicoswilson on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
playdohface
left a comment
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.
A few points here:
- I am not sure the semantics of
check_updates_on_startupare ideal. It should probably have optionsonce|never|always, whereoncemeans do the download if you have nothing but don't update the versions. - Whatever semantics and names we agree on, they should be documented in the README
- Some of the comments are just stating the obvious and not helpful, i'd rather read the code
|
As discussed in #125, this approach of allowing users to specify the version they want to download makes the code more complex and, in my opinion, difficult to maintain. I would prefer the option to provide a file path instead of downloading something for the user. The idea about updates could be improved, as suggested by @playdohface. The current implementation for downloading components requires an overall first step before adopting such a feature. Currently, things are scattered. We would likely have a single component that, when the extension starts, checks for updates and manages downloads. |
|
@playdohface I agree with your idea about check_updates_on_startup. My initial reason for designing this feature was the network environment in China — sometimes checking for updates can hang for a long time and block normal startup. Let’s go with your proposed approach, and we can document it properly in the README. |
|
@tartarughina |
Replaces per-component update check flags and custom URLs with a unified `update_check_mode` configuration supporting 'always', 'once', and 'never' modes. Introduces a shared resolver utility for component installation logic, simplifying and unifying how JDTLS, Lombok, and Debugger are managed. Updates documentation and cleans up related code paths.
|
@tartarughina Due to the sandbox mechanism, I don't think it's necessary to provide a file path. |
playdohface
left a comment
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.
If we don't let the user supply the file-paths, how will they know where to put the files if they want to manage them manually?
|
@playdohface Thanks for the careful review! |
tartarughina
left a comment
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 the general approach and idea brought forward by this PR although I think we are missing an opportunity of having a single component that handles updates, downloads and eventually manages user provide paths.
For this reason, before going blindly into new revisions, let's come up with a proper design. We have #125 which is a good place where to continue the conversation. I'm not asking for anything fancy and corporate level, a sketch on Excalidraw is perfectly fine.
playdohface
left a comment
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 think generally this looks fine now, I left some nitpicks. I haven't had the time to test the behaviour yet I'll do that later today and if it works I think we can land this and then work on letting users supply the file paths in a different PR
Add configuration options to allow users to specify custom paths for JDTLS, Lombok, and Java Debug components instead of relying on automatic downloads. Changes: - Add get_jdtls_launcher(), get_lombok_jar(), and get_java_debug_jar() config helpers - Support jdtls_launcher, lombok_jar, and java_debug_jar settings - User-provided paths take precedence over managed installations - Update check_updates behavior: ignored for components with custom paths - Refactor component resolution logic for consistent precedence handling Documentation improvements: - Restructure README with dedicated Configuration Options section - Add detailed explanation of check_updates modes and behavior - Document new custom path options with clear examples - Fix typos and improve grammar throughout - Enhance code comments for better clarity
New Configuration Options:
check_updates: Controls when to check for updates for JDTLS, Lombok, and Java Debug components"always"(default): Always check for and download the latest version on every startup"once": Check for updates only if no local installation exists"never": Never check for updates, only use existing local installations (will error if missing)"always"jdtls_launcher: Path to a custom JDTLS launcher scriptcheck_updatesis ignored for JDTLSlombok_jar: Path to a custom Lombok JAR filecheck_updatesis ignored for Lombokjava_debug_jar: Path to a custom Java Debug plugin JAR filecheck_updatesis ignored for the debuggerBehavior:
~/path/to/jar)