Skip to content
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

Add a velocity template based generator #269

Merged
merged 12 commits into from
Dec 29, 2022
Merged

Conversation

@sonatype-lift
Copy link

sonatype-lift bot commented Oct 23, 2022

⚠️ 14 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

@hboutemy
Copy link
Member

hboutemy commented Oct 26, 2022

I'm trying to guess what this does Modello generator does, to be able to at least describe it before going more in depth into documenting it

it was created in Maven, used for Maven model:

@hboutemy
Copy link
Member

hboutemy commented Oct 26, 2022

it's not clear to me why this is a new Maven plugin instead of a Modello plugin that can be used by Modello Maven plugin https://codehaus-plexus.github.io/modello/

@gnodet
Copy link
Member Author

gnodet commented Oct 26, 2022

it's not clear to me why this is a new Maven plugin instead of a Modello plugin that can be used by Modello Maven plugin https://codehaus-plexus.github.io/modello/

It's not a new maven plugin, but s a new modello plugin. It is used with the modello maven plugin. See https://github.com/apache/maven/pull/848/files to see how it will be used.
In maven 4.x, it is currently a standalone plugin, but if we want to integrate it into the modello, a modello plugin makes more sense, and that's what this PR is about.

@gnodet
Copy link
Member Author

gnodet commented Oct 26, 2022

I've added some javadocs.
Note that using the velocity generator is more like writing a generator. And modello has not much documentation about how to use its model. The Helper helps a bit for accessing a few common things.

gnodet and others added 4 commits October 26, 2022 16:41
…ModelloVelocityMojo.java

Co-authored-by: Konrad Windszus <konrad_w@gmx.de>
… them relative to the basedir so that Velocity can find them
@gnodet gnodet changed the title Add a velocity template based plugin Add a velocity template based generator plugin Oct 26, 2022
@gnodet gnodet changed the title Add a velocity template based generator plugin Add a velocity template based generator Oct 26, 2022
Copy link
Contributor

@kwin kwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the updates. LGTM now.

@gnodet
Copy link
Member Author

gnodet commented Oct 27, 2022

@hboutemy are you ok ?

* {@code org.apache.maven.api.model} and the variable {@code className} is set to {@code Plugin}.
* <p>
* {@code #MODELLO-VELOCITY#REDIRECT ${package.replace('.','/')}/${className}.java}
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnodet nice documentation, I'm starting to understand
adding an integration test in https://github.com/codehaus-plexus/modello/tree/master/modello-maven-plugin/src/it will help showing classical use cases

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnodet looking at the only use case I know of https://github.com/apache/maven/tree/master/maven-model/src/main/mdo
I'm worried on the use case: do you really want to copy paste all the generation code from Modello and tweak it by hand?
I understand that it's easy to prototype some updates to the generated java classes, but I fear it will be unmaintainable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are mostly new generators not supported by modello. The v4 model is immutable which is not supported, the v3 wraps the immutable v4 model, which is not supported, the reader returns the v4 immutable model , which is not supported, the transformer did not exist in maven 3.x, and the merger was hand written... So that's not tweaking the modello code, those are really new generators.
So given those are new generators, we'd have to reimplement them all in modello.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, there's still a problem with the current state in that the templates are duplicated instead of being reused. This need to be fixed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had started working on an IT a few weeks ago, but my laptop died with the unfinished work on it (and I can't even turn that laptop on anymore....). I'll see if I can give in another try in the coming weeks.

* <li>{@code Helper}: a {@link org.codehaus.modello.plugin.velocity.Helper} object instance</li>
* <li>any additional parameters specified using the {@link #params} property</li>
* </ul>
* The output file is controlled from within the template using the {@code #MODELLO-VELOCITY#REDIRECT}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the term "redirect" confusing: makes me think HTTP redirect.
Can we rename to something like "OUTPUT"?

@hboutemy
Copy link
Member

I added documentation: it helps me understand the feature sufficiently to do the release (let's consider Maven core usage is an IT for now, I'll see later about making an internal IT)

the only little thing that I would love to change is #MODELLO-VELOCITY#REDIRECT: #MODELLO-VELOCITY#SAVE-OUTPUT-TO?

@michael-o
Copy link
Member

What is the benefit of this over the other components?

@gnodet
Copy link
Member Author

gnodet commented Dec 27, 2022

What is the benefit of this over the other components?

Ease of maintenance. Each generator can be easily replaced by a simple template that is easy to understand and modify. Also, the user can easy author or modify templates, while existing generators can not.

@michael-o
Copy link
Member

What is the benefit of this over the other components?

Ease of maintenance. Each generator can be easily replaced by a simple template that is easy to understand and modify. Also, the user can easy author or modify templates, while existing generators can not.

So this is a replacement for the Java code?

@gnodet
Copy link
Member Author

gnodet commented Dec 27, 2022

What is the benefit of this over the other components?

Ease of maintenance. Each generator can be easily replaced by a simple template that is easy to understand and modify. Also, the user can easy author or modify templates, while existing generators can not.

So this is a replacement for the Java code?

Not really, unless we ship the VTL templates and lock them. The current code is in maven master and was needed in order to be able to generate the v4 model, the v4 readers, the v3 models wrapping the v4 models, the mergers, etc... I originally started by hacking the current generators, but it was too complicated and time consuming, so I ended up writing this template generator. So all the generators could be rewritten based on a template I suppose, but this PR is here due to @hboutemy request to move it from the maven master branch to the modello project where it actually belongs.

@hboutemy
Copy link
Member

hboutemy commented Dec 27, 2022

So this is a replacement for the Java code?

yes and no

yes, it is an ultra flexible replacement for the classical generated code by existing Velocity plugins, because the new immutable Maven API creates new constraints with its non-immutable part: I'm also describing the way this new Maven API is built apache/maven#830

but in fact, this Velocity plugin ships no real code generator, the code is in the Velocity templates: each user can choose to use the classical Velocity plugins to get classical fully-Modello generated Java code (we don't remove the classical code generators), or they can choose to create their own Velocity templates to generate the code they want

In Maven core, the new Maven API forced Guillaume to create new code generators with custom Velocity templates https://github.com/apache/maven/tree/c07700ffc97d3967a8de77d5a1065f5e6115c00e/maven-model/src/main/mdo , but classical Modello plugins are used to generate xdoc and xsd

To me, this Velocity plugin is ready to merge, we just need to rename #MODELLO-VELOCITY#REDIRECT to something like #MODELLO-VELOCITY#SAVE-OUTPUT-TO or any better idea welcome

@gnodet
Copy link
Member Author

gnodet commented Dec 29, 2022

To me, this Velocity plugin is ready to merge, we just need to rename #MODELLO-VELOCITY#REDIRECT to something like #MODELLO-VELOCITY#SAVE-OUTPUT-TO or any better idea welcome

I don't have commit rights in this repo, so I'm not able to merge it.

@kwin kwin merged commit 394178a into codehaus-plexus:master Dec 29, 2022
@kwin
Copy link
Contributor

kwin commented Dec 29, 2022

I don't have commit rights in this repo, so I'm not able to merge it.

Just squashed and merged it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants