-
Notifications
You must be signed in to change notification settings - Fork 35
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
Issue 215: Use API defaults for a simpler experience #235
base: main
Are you sure you want to change the base?
Conversation
d49bb2e
to
6712f3d
Compare
…pi default values
6712f3d
to
bbc31a5
Compare
I looked at my previous commits and realized they were not signed, for some reason. Had to force-push to fix that. |
@jdneo @testforstephen this PR has been waiting for a review for quite a while, could you take a look? |
const items = await serviceManager.getItems(projectMetadata.serviceUrl, MetadataType.BOOTVERSION); | ||
|
||
if (projectMetadata.enableSmartDefaults === true) { | ||
projectMetadata.bootVersion = items.find(x => x.default === true)?.value?.id; |
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 should guard the case of projectMetadata.bootVersion
is null, and fall back to the prompt mode.
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.
Fixed that for every API default. Thanks!
I prefer to use the configured default values for |
@testforstephen regarding the use of already present At least that's my goal in this example from const javaVersion: string = projectMetadata.defaults.javaVersion || workspace.getConfiguration("spring.initializr").get<string>("defaultJavaVersion");
if (javaVersion) {
projectMetadata.javaVersion = javaVersion;
// this finishes the step execution and uses the default set by user with already existing configuration
return true;
}
const items = await serviceManager.getItems(projectMetadata.serviceUrl, MetadataType.JAVAVERSION);
if (projectMetadata.enableSmartDefaults === true) {
projectMetadata.javaVersion = items.find(x => x.default === true)?.value?.id;
// new code: use the api default if existing default configuration is not set
return true;
} Improvement ideaPerhaps a better name for the setting would be
Let me know what you think! And I appreciate any additional feedback, of course. |
Since we already allow users to specify default value for certain settings (such as Java version, language packing), and won't display them in the Creation Wizard if specified, this approach might be enough. One concern with relying on the API's default values is the lack of transparency and predictability. It's not clear to me that which default values are being applied, and these defaults may change over time. |
Add @martinlippert (the lead of the spring tools) for comments, what do you think of the user experience in this PR? |
Using defaults from the API makes sense to me. The overall sequence of picking the default values seems good to me as well:
We could also think about remembering the selected values (in the same way we do this for the selected dependencies). A difficult part here is to have up-to-date values in the preference dialog. At the moment, the values are extremely outdated (take a look at the java version drop down, for example). So it would be best if we could dynamically define the values for the drop downs in the preferences ans populate them with choices from the API. Another overall difficulty here is that we need to make sure that - whatever the API decides the default values should be - the overall IDE experience should nicely support (e.g. the version of Java). If - for example - the API decides to switch to Kotlin as the preferred language (I don't see that coming, but just as an example here), we should not show that as the default value in the IDE for as long as we don't have good Kotlin language tooling in Code. |
Thanks for the comments, @testforstephen and @martinlippert! Firstly, I agree with the order for the preferences, that should be the result of this PR. Remembering last used values for packaging, Java version and etc should be possible, but I believe a separate PR would be more appropriate for such change. As for the outdated recommendations in the extension settings, those are statically set in the For now, based on my current free time, I would like to limit this PR only to using the API defaults on an opt-in basis. I will also change the setting name to As the API does not change drastically too often, there is time to explore the discussed improvements later, and I'll be glad to open additional PRs if I'm able to. |
Yes, please go ahead. We need to rename the setting to give more clarification on what it does.
We can write a utility js for this task, and run the script to automatically update the package.json with the new API values. This script doesn't need to be integrated into the CI job and can be run manually on a periodic basis. |
Hello, @testforstephen. Just pushed the change I mentioned. Guess that's it for this PR! |
New setting to allow for Spring Initializr recommended defaults to be used for following configurations:
Setting:
spring.initializr.useApiDefaults
*Type: boolean
Default value:
false
The default values will be based on the default field from API's response, such as below:
If there's a default value set via existing configurations such as
defaultJavaVersion
, then those take precedence over the API recommendation.If the API response does not contain a default value for a property, then the existing behavior applies and the extension prompts the user to select one.
Closes #215