feat: Catalog templates with property list #1205

nandorholozsnyak · 2022-01-18T23:02:06Z

Property can have description and default value
Adding multiple property at the same time with a specific format
Listing templates returns the list of the template properties too

Associated issue: #1203

- Property can have description and default value - Adding multiple property at the same time with a specific format - Listing templates returns the list of the template properties too

quintesse

Looks good! Just needs a couple of minor changes.
It also needs tests and changes to the docs :-)

quintesse · 2022-01-19T09:57:00Z

src/main/java/dev/jbang/cli/TemplatePropertyConverter.java

+	public TemplateAdd.TemplatePropertyInput convert(final String input) throws Exception {
+		String[] split = input.split(":");
+		TemplateAdd.TemplatePropertyInput templatePropertyInput = new TemplateAdd.TemplatePropertyInput();
+		if (split.length > 0) {


I think this needs checking that the value isn't empty

quintesse · 2022-01-19T09:58:02Z

src/main/java/dev/jbang/cli/TemplatePropertyConverter.java

+		if (split.length > 0) {
+			templatePropertyInput.setKey(split[0]);
+		}
+		if (split.length > 1) {


And this probably shouldn't set the value if the split value is empty (so foo::bar would skip setting the description)

quintesse · 2022-01-19T09:58:21Z

src/main/java/dev/jbang/cli/TemplatePropertyConverter.java

+		if (split.length > 1) {
+			templatePropertyInput.setDescription(split[1]);
+		}
+		if (split.length > 2) {


Same here, skip if empty.

quintesse · 2022-01-19T10:00:00Z

src/main/java/dev/jbang/catalog/Template.java


-	public Template(Map<String, String> fileRefs, String description, Catalog catalog) {
+	public Template(Map<String, String> fileRefs, String description, Catalog catalog,


This is not wrong of course, but could you change the order a bit? The catalog is meant to be the last argument in the list.

quintesse · 2022-01-19T10:01:45Z

src/main/java/dev/jbang/catalog/Catalog.java

@@ -59,7 +59,7 @@ public Catalog(String baseRef, String description, ResourceRef catalogRef, Map<S
 				new Alias(a.scriptRef, a.description, a.arguments, a.javaOptions, a.dependencies, a.repositories,
 						a.classpaths, a.properties, a.javaVersion, a.mainClass, this)));
 		templates.forEach((key, t) -> this.templates.put(key,
-				new Template(t.fileRefs, t.description, this)));
+				new Template(t.fileRefs, t.description, this, new HashMap<>())));


Use Collections.emptyMap() here.

quintesse · 2022-01-19T10:05:33Z

src/main/java/dev/jbang/catalog/CatalogUtil.java

-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
+import java.util.*;


Make sure you run ./gradlew spotlessApply before committing, no * imports are allowed.
I always run ./gradlew spotlessApply clean build installDist to make sure all my code has been cleaned up.

quintesse · 2022-01-19T10:06:03Z

src/main/java/dev/jbang/catalog/CatalogUtil.java

@@ -120,22 +117,25 @@ private static void removeAlias(Catalog catalog, String name) {
 	/**
 	 * Adds a new template to the nearest catalog
 	 *
-	 * @param name The name of the new template
+	 * @param properties


Could you add some description here?

quintesse · 2022-01-19T10:06:16Z

src/main/java/dev/jbang/catalog/CatalogUtil.java

 		return catalogFile;
 	}

 	/**
 	 * Adds a new template to the given catalog
 	 *
+	 * @param properties


quintesse · 2022-01-19T10:09:20Z

src/main/java/dev/jbang/cli/Template.java

+																		.append(Util.repeat(" ", indent + 4))
+																		.append(ConsoleOutput.cyan(entry.getKey()))
+																		.append(" = ");
+				if (StringUtils.isNotBlank(entry.getValue().getDescription())) {


Given the changes I suggested for the TemplatePropertyConverter I'd simply check for null here.

quintesse · 2022-01-19T10:12:12Z

src/main/java/dev/jbang/catalog/TemplateProperty.java

+public class TemplateProperty {
+
+	private String description;
+	@SerializedName(value = "default-value")


This is just my personal preference but I'd suggest simply using default instead of default-value.

nandorholozsnyak · 2022-01-19T21:34:06Z

Thank you for the feedback @quintesse . Will take a look on these.
Just a quick feedback, in the Contribution file maybe some code standard and requirements should be visible, I understand I have to write the tests and make sure it is well formatted, but for the formatting issues I do remember I ran the spotless plugin before committing :D I'm just setting up the git hook for it to never happen again.
Is there any proper documentation about the code layering and structure?

- Changes based on PR requests - Test for TemplatePropertyConverter

quintesse

This would already be good enough to merge IMO.
But we need a couple of more tests, especially in TestTemplate, just to make sure that the information gets stored and retrieved correctly. A couple of edge cases perhaps (wrongly formatted property, things like that).

quintesse · 2022-01-20T00:33:37Z

Is there any proper documentation about the code layering and structure?

Not really, I'd just look at the surrounding code when you add/change things and try to follow that style as much as possible. And if you write something completely new then we tend to just let you write it as you want, as long as it is good code :-)

quintesse · 2022-01-20T00:36:35Z

This would already be good enough to merge IMO.

Once it builds and tests correctly of course 😉

maxandersen · 2022-01-20T00:41:26Z

Is there any proper documentation about the code layering and structure?

Not really, I'd just look at the surrounding code when you add/change things and try to follow that style as much as possible. And if you write something completely new then we tend to just let you write it as you want, as long as it is good code :-)

...and we apply code formatting automatically so the way I code is just whatever I need with whatever formatting my ide uses - Then run the spotlessapply
And it will fix it. Avoids having to think about formatting :)

quintesse · 2022-01-20T00:49:19Z

And it will fix it

We should do something about the indenting, though. The fact that it tries to line up with the end of the first line (or whatever it is it tries to do) often creates some pretty horrible indents 😞

nandorholozsnyak · 2022-01-20T07:56:33Z

I have never used Spotless before but .editorconfig. For me it was working well, of course if you are an Eclipse or Netbeans user you need a plugin for it in the IDE, but it has Gradle plugins as well.
https://editorconfig.org/

quintesse · 2022-01-20T09:04:19Z

@nandorholozsnyak Oh, Spotless works well enough, it's just the chosen configuration that I'm somewhat unhappy with :-)

codecov · 2022-01-23T10:12:11Z

Codecov Report

Merging #1205 (e64038d) into main (8919f79) will decrease coverage by 0.11%.
The diff coverage is 44.94%.

❗ Current head e64038d differs from pull request most recent head 11a01e6. Consider uploading reports for the commit 11a01e6 to get more accurate results

@@             Coverage Diff              @@
##               main    #1205      +/-   ##
============================================
- Coverage     58.96%   58.84%   -0.12%     
- Complexity     1049     1057       +8     
============================================
  Files            84       86       +2     
  Lines          5578     5659      +81     
  Branches        937      954      +17     
============================================
+ Hits           3289     3330      +41     
- Misses         1806     1837      +31     
- Partials        483      492       +9

Flag	Coverage Δ
Linux	`57.69% <44.94%> (-0.11%)`	⬇️
Windows	`57.60% <44.94%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/main/java/dev/jbang/catalog/Catalog.java	`70.00% <ø> (ø)`
src/main/java/dev/jbang/cli/Catalog.java	`32.38% <0.00%> (ø)`
.../main/java/dev/jbang/catalog/TemplateProperty.java	`27.77% <27.77%> (ø)`
src/main/java/dev/jbang/cli/Template.java	`47.61% <43.85%> (-0.21%)`	⬇️
src/main/java/dev/jbang/catalog/CatalogUtil.java	`61.78% <50.00%> (ø)`
.../java/dev/jbang/cli/TemplatePropertyConverter.java	`80.00% <80.00%> (ø)`
src/main/java/dev/jbang/catalog/Template.java	`48.00% <100.00%> (+2.16%)`	⬆️
src/main/java/dev/jbang/cli/App.java	`51.93% <0.00%> (+0.20%)`	⬆️
src/main/java/dev/jbang/source/JarSource.java	`79.06% <0.00%> (+4.65%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82de0ae...11a01e6. Read the comment docs.

nandorholozsnyak · 2022-01-23T11:17:50Z

Hey @quintesse I added tests and small changes. I added the CLI docs as well, but I'm not sure if it would be enough or not.

quintesse

Code looks good! Please run ./gradlew spotlessApply and re-push because that's what making the CI build fail right now.

…command test: Test for template properties adding and reading fix: Template properties NPE check fix: Spotless applied

nandorholozsnyak · 2022-01-24T21:14:58Z

Btw, locally I use Java 17 (Temurin) and the build fails with that, so this is why I was not able to spot what is not working, Spotless is not working with that, or at least some things are not okay with that.
I ran the code you mentioned, now it should be working.

quintesse · 2022-01-24T22:55:31Z

Good job @nandorholozsnyak, thanks !

nandorholozsnyak · 2022-01-26T17:42:14Z

Just a quick question @quintesse , did I just screw up the Codecov reports? I see that the master is failing because of the codecov report, and I just saw your new PR and checked the code as well and saw a lot of "misses" by the codecov. Should I open a PR to fix those?

quintesse · 2022-01-26T23:30:15Z

Codecov is still in somewhat of an experimental phase for us right now, so don't worry too much about it. At some point we'll want to do something with that information though :-)

feat: Catalog templates with property list

03b826d

- Property can have description and default value - Adding multiple property at the same time with a specific format - Listing templates returns the list of the template properties too

quintesse requested changes Jan 19, 2022

View reviewed changes

feat: Catalog templates with property list

69abd5e

- Changes based on PR requests - Test for TemplatePropertyConverter

quintesse previously approved these changes Jan 20, 2022

View reviewed changes

nandorholozsnyak dismissed quintesse’s stale review via 39426e4 January 23, 2022 10:07

nandorholozsnyak marked this pull request as ready for review January 23, 2022 11:17

nandorholozsnyak force-pushed the jbang-catalog-template-properties branch from ef42f8d to a223b8d Compare January 23, 2022 21:23

quintesse requested changes Jan 24, 2022

View reviewed changes

docs: Documentation for -P and --property option in template add cli …

11a01e6

…command test: Test for template properties adding and reading fix: Template properties NPE check fix: Spotless applied

nandorholozsnyak force-pushed the jbang-catalog-template-properties branch from e64038d to 11a01e6 Compare January 24, 2022 21:11

quintesse approved these changes Jan 24, 2022

View reviewed changes

quintesse merged commit 106be11 into jbangdev:main Jan 24, 2022

quintesse mentioned this pull request Jan 24, 2022

Extending templates with properties in catalog #1203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Catalog templates with property list #1205

feat: Catalog templates with property list #1205

nandorholozsnyak commented Jan 18, 2022

quintesse left a comment

quintesse Jan 19, 2022

quintesse Jan 19, 2022

quintesse Jan 19, 2022

quintesse Jan 19, 2022

quintesse Jan 19, 2022

quintesse Jan 19, 2022

quintesse Jan 19, 2022

quintesse Jan 19, 2022

quintesse Jan 19, 2022

quintesse Jan 19, 2022

nandorholozsnyak commented Jan 19, 2022

quintesse left a comment

quintesse commented Jan 20, 2022

quintesse commented Jan 20, 2022

maxandersen commented Jan 20, 2022

quintesse commented Jan 20, 2022

nandorholozsnyak commented Jan 20, 2022

quintesse commented Jan 20, 2022

codecov bot commented Jan 23, 2022 •

edited

Loading

nandorholozsnyak commented Jan 23, 2022

quintesse left a comment

nandorholozsnyak commented Jan 24, 2022

quintesse commented Jan 24, 2022

nandorholozsnyak commented Jan 26, 2022

quintesse commented Jan 26, 2022


		public Template(Map<String, String> fileRefs, String description, Catalog catalog) {
		public Template(Map<String, String> fileRefs, String description, Catalog catalog,

feat: Catalog templates with property list #1205

feat: Catalog templates with property list #1205

Conversation

nandorholozsnyak commented Jan 18, 2022

quintesse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nandorholozsnyak commented Jan 19, 2022

quintesse left a comment

Choose a reason for hiding this comment

quintesse commented Jan 20, 2022

quintesse commented Jan 20, 2022

maxandersen commented Jan 20, 2022

quintesse commented Jan 20, 2022

nandorholozsnyak commented Jan 20, 2022

quintesse commented Jan 20, 2022

codecov bot commented Jan 23, 2022 • edited Loading

Codecov Report

nandorholozsnyak commented Jan 23, 2022

quintesse left a comment

Choose a reason for hiding this comment

nandorholozsnyak commented Jan 24, 2022

quintesse commented Jan 24, 2022

nandorholozsnyak commented Jan 26, 2022

quintesse commented Jan 26, 2022

codecov bot commented Jan 23, 2022 •

edited

Loading