-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Make templates adjust with scala version #25293
Conversation
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.
Thanks! Added a comment.
...base-codestarts/src/main/resources/codestarts/quarkus/buildtool/maven/scala/pom.tpl.qute.xml
Outdated
Show resolved
Hide resolved
...starts/src/main/java/io/quarkus/devtools/codestarts/core/reader/QuteCodestartFileReader.java
Outdated
Show resolved
Hide resolved
Are those methods added so it does not break past versions of cli running this as they won't have access to those do they? |
} | ||
|
||
@Override | ||
public CompletionStage<Object> resolve(EvalContext context) { |
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.
Can't we call methods on string anyway ?
@@ -52,7 +52,12 @@ | |||
<arg>-deprecation</arg> | |||
<arg>-feature</arg> | |||
<arg>-explaintypes</arg> | |||
{#if scala.version.startsWith("2.12.")} |
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.
Is that really the exact version to use ? What if we upgrade to 2.13 ?
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 upgraded to 2.13, that's the issue. Old versions need a different path.
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.
Yes I get that but can't we make the template more open ended so it works with 2.13 and higher so we avoid having to keep updating ?
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.
@maxandersen we could make it configurable I guess and check if the variable exists, maybe that's the best backward/forward compatible fix.
Yeah looks like this will be a problem indeed. |
This comment has been minimized.
This comment has been minimized.
Yes :-/ Not sure how we can deal with this.. beside hard coding the version check.. |
I believe maintaining CLI forward compatibility is a burden that we could avoid! |
We should provide an easy CLI upgrade method and lock versions compatibility, or at least have a mechanism to be able to break and ask the user to upgrade on new features. We lock CLI compat forward on the same minor and backward for all. When a new minor/major is out, we tell the user that a new version is out and to upgrade the CLI. |
Sorry I was tired yesterday, this fix is ok as the "base codestarts" (which are more coupled than the other extensions codestarts) are tied to the generator exactly for this reason (The previous CLI will use the previous base codestarts templates) I will fix the tests. |
I 100% disagree. It's what we should enable as much as possible since it's very doable but we should not be absolutist about it. So we and must try and minimize impact. if we force upgrade cli you make it harder to explore as users will start preferring not to try upgrading as breaks their current workflows. |
Okey that's good. But we should still do what we can to enable non core extensions to be able to work or at least guard when used with different version of cli. |
ah snap wrong button |
The template is now different depending on the given scala version.
I added the possibility to use
startsWith
,endsWith
andcontains
on String in codestarts templates thanks to @mkouba :)