Reformat the templates for Kotlin model code #310

MariusVolkhart · 2024-01-07T16:57:33Z

Change import statements to not have an indent. No functional change.
Remove unused imports. These were likely copied over from the Java template, but are not needed in Kotlin.
Dynamic methods, which generate a map, now use Kotlin's buildMap() instead of mapOf(). mapOf() produces extra garbage, as it creates a Pair for each entry, whereas buildMap() modifies the underlying map directly.
Static methods are now defined as an expression. No functional change.
In static methods, the template class name is no longer fully qualified, as it's provided as an import. No functional change.
Remove semicolons - idiomatic Kotlin doesn't use them. No functional change.

edward3h

Looks good.

casid · 2024-01-08T07:11:11Z

@MariusVolkhart can you have a look at the failing tests? I believe they need to be adjusted to fit the improved code style?

MariusVolkhart · 2024-01-10T04:39:15Z

I don't know what to make of these test failures. The lines they point to are comments, and the error message they give isn't helping me - Collectors is definitely imported. test target runs locally, and I know "CI don't lie 😃 )", but this one is new for me 😅

casid · 2024-01-10T05:31:20Z

That's really weird, it compiles locally here as well.

@marcospereira could this be an issue with the CI caching introduced recently?

marcospereira · 2024-01-10T07:26:02Z

@marcospereira could this be an issue with the CI caching introduced recently?

It is possible since jte build publishes artifacts locally, and I guess those are being included in the cache. I will do some investigation tomorrow, but a way to validate if the caches are screwing the builds here is to delete all of them (using gh cli), and trigger a new build manually (for this pr).

marcospereira · 2024-01-10T19:58:26Z

I'm unsure if cache was the culprit here, or if this was just a CI fluke. Still, I created #316 since it would be good hygiene.

@casid, can you trigger another build, and if it fails, delete the caches, and trigger another one? It would at least give us some confirmation if the failure is related to caching artifacts.

marcospereira · 2024-01-10T20:06:31Z

jte-models/src/main/jte/statictemplates/kmain.jte

-import gg.jte.TemplateOutput
-import gg.jte.html.HtmlInterceptor


I'm not 100% sure about this removal.

There is a call to kmethod a few lines below:

jte/jte-models/src/main/jte/statictemplates/kmain.jte

Line 24 in 31fc79d

@template.statictemplates.kmethod(config = config, template = template)

And that template later declares this variable:

jte/jte-models/src/main/jte/statictemplates/kmethod.jte

Line 8 in 31fc79d

!{String outputClass = config.contentType() == ContentType.Html ? "HtmlTemplateOutput" : "TemplateOutput";}

That is then used by StaticJteModel in the next line:

jte/jte-models/src/main/jte/statictemplates/kmethod.jte

Line 9 in 31fc79d

override fun ${Util.methodName(template)}(${Util.kotlinTypedParams(template, false)}): JteModel = StaticJteModel<${outputClass}>(

Yup! I'd say it's needed. Thanks for catching that

casid · 2024-01-11T05:56:39Z

@marcospereira I re-ran the ubuntu build yesterday and it still failed. https://github.com/casid/jte/actions/runs/7470236001/job/20359418271?pr=310

will try to figure out how to clear the cache.

marcospereira · 2024-01-11T06:21:00Z

will try to figure out how to clear the cache.

I made a comment about it in #316. If you have GH CLI installed, you only need to run gh cache delete --repo casid/jte --all.

casid · 2024-01-11T07:24:28Z

Thanks! I just cleared the cache via gh cli and reran the tests this morning, but the problem remains (https://github.com/casid/jte/actions/runs/7470236001/job/20372467503?pr=310). So this is not a cache problem.

marcospereira · 2024-01-16T15:49:22Z

tl;dr

There is a mismatch between your branch and the PR changes. A possible fix is to amend your last commit, force push, and see if this fixes the issue:

git commit --amend --no-edit
git push --force-with-lease

🤞🏼

What is going on?

test target runs locally, and I know "CI don't lie 😃 )", but this one is new for me 😅

We got tricked by this. CI is not running test target, but instead install. I was able to reproduce the issue locally after checking out your branch, and running the same command CI runs:

gh pr checkout 310
./mvnw clean install --file pom.xml

The lines they point to are comments, and the error message they give isn't helping me - Collectors is definitely imported

Well, it is not. 🙈

This is the main branch right now:

jte/jte-models/src/test/java/gg/jte/models/generator/TestModelExtension.java

Lines 1 to 29 in 7ad2e12

    
           package gg.jte.models.generator; 
        
           import gg.jte.ContentType; 
        
           import gg.jte.extension.api.JteConfig; 
        
           import gg.jte.extension.api.JteExtension; 
        
           import gg.jte.extension.api.TemplateDescription; 
        
           import org.junit.jupiter.api.DisplayName; 
        
           import org.junit.jupiter.api.Nested; 
        
           import org.junit.jupiter.api.Test; 
        
           import java.io.IOException; 
        
           import java.nio.file.Files; 
        
           import java.nio.file.Path; 
        
           import java.nio.file.Paths; 
        
           import java.util.Collection; 
        
           import java.util.Collections; 
        
           import java.util.HashMap; 
        
           import java.util.List; 
        
           import java.util.Map; 
        
           import java.util.Optional; 
        
           import java.util.regex.Pattern; 
        
           import static gg.jte.extension.api.mocks.MockConfig.mockConfig; 
        
           import static gg.jte.extension.api.mocks.MockParamDescription.mockParamDescription; 
        
           import static gg.jte.extension.api.mocks.MockTemplateDescription.mockTemplateDescription; 
        
           import static org.assertj.core.api.Assertions.assertThat; 
        
           import static org.junit.jupiter.api.Assertions.assertEquals; 
        
           import static org.junit.jupiter.api.Assertions.fail;

And your PR IS NOT adding an import for Collectors:

https://github.com/casid/jte/pull/310/files#diff-96170913ebec3cdf00a5e705cb673c330e9180692872b503de8c67c55a74e364

See? No imports there! 👆🏼

Now, here is the WTH part of this. When I navigate to your branch, the import is there:

https://github.com/MariusVolkhart/jte/blob/mv/kotlinTemplates/jte-models/src/test/java/gg/jte/models/generator/TestModelExtension.java#L22

Permalink:

https://github.com/MariusVolkhart/jte/blob/31fc79df1f1a0ae59fe1e977c350e6b1aa1c5654/jte-models/src/test/java/gg/jte/models/generator/TestModelExtension.java#L22

I guess that you run into some weird sync issues between your branch and the PR state? Maybe something related to this:

https://github.com/orgs/community/discussions/16351

marcospereira · 2024-01-16T15:59:44Z

Okay, I got what is happening now.

This is where the import for Collectors was removed: #311.

But your branch is not up to date with that change. You need to rebase it on top of casid/jte main branch, solve the issues in the code, and then push-force here.

marcospereira · 2024-01-16T16:01:38Z

@casid, it may be a good idea to enable this configuration:

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-suggestions-to-update-pull-request-branches

It will be clear when the PR branch is outdated, and help to fix confusions like this.

casid · 2024-01-16T16:38:30Z

Thanks for the investigation @marcospereira!

@casid, it may be a good idea to enable this configuration:

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-suggestions-to-update-pull-request-branches

It will be clear when the PR branch is outdated, and help to fix confusions like this.

This setting makes a lot of sense, I've enabled it and will give the button a try :-).

marcospereira · 2024-01-16T16:48:22Z

This setting makes a lot of sense, I've enabled it and will give the button a try :-).

Thanks!

So, the build is still failing because, now that the branch is rebased on top of the main, it does not include the import, and it should be failing locally the same way CI fails (since the Collectors import is missing everywhere now).

marcospereira · 2024-01-24T20:02:55Z

Hey @casid @MariusVolkhart,

I would love to see a new release containing #319, and I think the changes here can be part of it since this PR is virtually done.

@MariusVolkhart, if you don't have the time, I can create a separate pull request with your commits (keeping proper attribution, of course) and fix the small issue blocking progress here.

Let me know if you are okay with that.

MariusVolkhart · 2024-01-24T20:29:22Z

@marcospereira Awesome! Thanks for the investigation! Like I said, CI don't lie 😛

And thanks for the bump on this. Looking at it now.

- Change import statements to not have an indent. No functional change. - Remove unused imports. These were likely copied over from the Java template, but are not needed in Kotlin. - Dynamic methods, which generate a map, now use Kotlin's buildMap() instead of mapOf(). mapOf() produces extra garbage, as it creates a Pair for each entry, whereas buildMap() modifies the underlying map directly. - Static methods are now defined as an expression. No functional change. - In static methods, the template class name is no longer fully qualified, as it's provided as an import. No functional change. - Remove semicolons - idiomatic Kotlin doesn't use them. No functional change.

Add import for Collectors

Restore the TemplateOutput import

codecov · 2024-01-24T21:31:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d4f82bf) 91.23% compared to head (3aa393e) 91.23%.

Additional details and impacted files

@@            Coverage Diff            @@
##               main     #310   +/-   ##
=========================================
  Coverage     91.23%   91.23%           
  Complexity     1216     1216           
=========================================
  Files            76       76           
  Lines          3159     3159           
  Branches        492      492           
=========================================
  Hits           2882     2882           
  Misses          164      164           
  Partials        113      113

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Adjust the tests for windows line endings

casid · 2024-01-25T05:49:56Z

Thank you guys! :-)

marcospereira · 2024-01-26T02:37:19Z

Thanks, @MariusVolkhart!

@casid, it would be great to have a 3.1.7 release if you have the time. :-)

casid · 2024-01-26T14:47:00Z

@marcospereira I just published 3.1.7 to maven central.

marcospereira · 2024-01-26T16:17:17Z

@marcospereira I just published 3.1.7 to maven central.

Thanks, @casid!

I tried it and... there is a bug in the model extension.

The change here:

https://github.com/casid/jte/pull/310/files#diff-6cd3488aa48299f9447bdf53f0733974078c6a61ce5cfdb107e7a829c3d3ea8fR14

It should use template.fullyQualifiedClassName() as it was before because you can have a template in a subfolder that will then generate a subpackage that is not added to the imports (which I think is the right thing to do).

I'm working on a quick fix and how to test for that.

Also, the changes in #310 are actually adding a number of unused imports in the model-extension facades. 🤔

/cc @MariusVolkhart.

marcospereira · 2024-01-26T16:32:41Z

Quickly documenting my findings. :-)

There is no Gradle test that combines:

Kotlin
The models extension
Used tags in subfolders

marcospereira · 2024-01-26T18:26:16Z

I'm working on a quick fix and how to test for that.

See #324.

MariusVolkhart · 2024-01-28T15:43:52Z

:( I’m sorry for that. Breaking things and adding more work for the maintainers. @marcospereira thanks for fixing it and adding new tests.

casid · 2024-01-28T15:47:40Z

@MariusVolkhart no worries! We have a saying in German, which goes like "those who do nothing, break nothing". I'm really proud that the detection and fix went so quickly and smooth. It's an incredible little community in this project.

marcospereira · 2024-01-28T16:18:34Z

We have a saying in German, which goes like "those who do nothing, break nothing".

Love this. Making and problems go together. It is how you react that matters. Keep the PRs coming, @MariusVolkhart.

It's an incredible little community in this project.

It is a small community, but the experience of contributing here is very welcoming. Thanks for always been so responsive, @casid.

edward3h approved these changes Jan 7, 2024

View reviewed changes

marcospereira mentioned this pull request Jan 10, 2024

ci: Do not cache locally published artifacts #316

Merged

marcospereira reviewed Jan 10, 2024

View reviewed changes

casid force-pushed the mv/kotlinTemplates branch from 31fc79d to 4848042 Compare January 16, 2024 16:38

MariusVolkhart added 4 commits January 24, 2024 15:29

fixup! Reformat the templates for Kotlin model code

95cf067

fixup! Reformat the templates for Kotlin model code

8782843

Add import for Collectors

fixup! Reformat the templates for Kotlin model code

5a7144a

Restore the TemplateOutput import

MariusVolkhart force-pushed the mv/kotlinTemplates branch from 4848042 to 5a7144a Compare January 24, 2024 20:43

fixup! Reformat the templates for Kotlin model code

e33fca2

fixup! Reformat the templates for Kotlin model code

3aa393e

Adjust the tests for windows line endings

casid merged commit 1a09cd6 into casid:main Jan 25, 2024
8 checks passed

MariusVolkhart deleted the mv/kotlinTemplates branch January 25, 2024 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reformat the templates for Kotlin model code #310

Reformat the templates for Kotlin model code #310

MariusVolkhart commented Jan 7, 2024

edward3h left a comment

casid commented Jan 8, 2024

MariusVolkhart commented Jan 10, 2024

casid commented Jan 10, 2024

marcospereira commented Jan 10, 2024

marcospereira commented Jan 10, 2024

marcospereira Jan 10, 2024

MariusVolkhart Jan 24, 2024

casid commented Jan 11, 2024

marcospereira commented Jan 11, 2024

casid commented Jan 11, 2024

marcospereira commented Jan 16, 2024 •

edited

Loading

marcospereira commented Jan 16, 2024

marcospereira commented Jan 16, 2024

casid commented Jan 16, 2024

marcospereira commented Jan 16, 2024

marcospereira commented Jan 24, 2024

MariusVolkhart commented Jan 24, 2024

codecov bot commented Jan 24, 2024 •

edited

Loading

casid commented Jan 25, 2024

marcospereira commented Jan 26, 2024

casid commented Jan 26, 2024

marcospereira commented Jan 26, 2024

marcospereira commented Jan 26, 2024

marcospereira commented Jan 26, 2024

MariusVolkhart commented Jan 28, 2024

casid commented Jan 28, 2024

marcospereira commented Jan 28, 2024

		import gg.jte.TemplateOutput
		import gg.jte.html.HtmlInterceptor

Reformat the templates for Kotlin model code #310

Reformat the templates for Kotlin model code #310

Conversation

MariusVolkhart commented Jan 7, 2024

edward3h left a comment

Choose a reason for hiding this comment

casid commented Jan 8, 2024

MariusVolkhart commented Jan 10, 2024

casid commented Jan 10, 2024

marcospereira commented Jan 10, 2024

marcospereira commented Jan 10, 2024

marcospereira Jan 10, 2024

Choose a reason for hiding this comment

MariusVolkhart Jan 24, 2024

Choose a reason for hiding this comment

casid commented Jan 11, 2024

marcospereira commented Jan 11, 2024

casid commented Jan 11, 2024

marcospereira commented Jan 16, 2024 • edited Loading

tl;dr

What is going on?

marcospereira commented Jan 16, 2024

marcospereira commented Jan 16, 2024

casid commented Jan 16, 2024

marcospereira commented Jan 16, 2024

marcospereira commented Jan 24, 2024

MariusVolkhart commented Jan 24, 2024

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

casid commented Jan 25, 2024

marcospereira commented Jan 26, 2024

casid commented Jan 26, 2024

marcospereira commented Jan 26, 2024

marcospereira commented Jan 26, 2024

marcospereira commented Jan 26, 2024

MariusVolkhart commented Jan 28, 2024

casid commented Jan 28, 2024

marcospereira commented Jan 28, 2024

marcospereira commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading