Reformat the templates for Kotlin model code

MariusVolkhart commented 6 months ago

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.

casid commented 6 months ago

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

MariusVolkhart commented 6 months ago

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 commented 6 months ago

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

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

marcospereira commented 6 months ago

@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 commented 6 months ago

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.

casid commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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:

https://github.com/casid/jte/blob/7ad2e12281ad6f11321a16515f3d793aa9e1b64d/jte-models/src/test/java/gg/jte/models/generator/TestModelExtension.java#L1-L29

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 commented 6 months ago

Okay, I got what is happening now.

This is where the import for Collectors was removed: https://github.com/casid/jte/pull/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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Hey @casid @MariusVolkhart,

I would love to see a new release containing https://github.com/casid/jte/pull/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 commented 6 months ago

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

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

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

casid commented 6 months ago

Thank you guys! :-)

marcospereira commented 6 months ago

Thanks, @MariusVolkhart!

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

casid commented 6 months ago

@marcospereira I just published 3.1.7 to maven central.

marcospereira commented 6 months ago

@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 commented 6 months ago

Quickly documenting my findings. :-)

There is no Gradle test that combines:

Kotlin
The models extension
Used tags in subfolders

marcospereira commented 6 months ago

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

See https://github.com/casid/jte/pull/324.

MariusVolkhart commented 6 months ago

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

casid commented 6 months ago

@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 commented 6 months ago

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.

casid / jte