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

Camel-freemarker component #835

Closed
wants to merge 30 commits into from

Conversation

carlosthe19916
Copy link

@carlosthe19916 carlosthe19916 commented Mar 2, 2020

Issue: #223

Added camel-freemarker component.
Changes:

  • The same test used for apache-camel-freemarker
  • We can add the list of locations where the templates can be found in the classpath using: quarkus.camel.freemarker.locations=myFolder1,myFolder2

Looking forward to have some feedback.

carlosthe19916 and others added 22 commits October 3, 2019 20:33
# Conflicts:
#	docs/modules/ROOT/pages/_partials/component-extensions.adoc
#	extensions/pom.xml
#	integration-tests/pom.xml
# Conflicts:
#	extensions/pom.xml
#	integration-tests/pom.xml
#	poms/bom-deployment/pom.xml
# Conflicts:
#	extensions/pom.xml
# Conflicts:
#	docs/modules/ROOT/pages/list-of-camel-quarkus-extensions.adoc
#	extensions/pom.xml
#	extensions/readme.adoc
#	poms/bom-deployment/pom.xml
# Conflicts:
#	docs/modules/ROOT/pages/list-of-camel-quarkus-extensions.adoc
#	extensions/pom.xml
#	extensions/readme.adoc
#	integration-tests/pom.xml
# Conflicts:
#	docs/modules/ROOT/pages/list-of-camel-quarkus-extensions.adoc
#	extensions/readme.adoc
#	integration-tests/pom.xml
Copy link
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

Please fix the codestyle by running 'mvn process-resources -Pformat'

@ppalaga ppalaga mentioned this pull request Mar 3, 2020
Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

Thanks, good work, I especially like the test coverage. A couple of questions and suggestions inline.

try {
List<String> locations = new ArrayList<>(camelFreemarkerConfig.locations);
if (locations.isEmpty()) {
locations.add("freemarker/templates");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please declare freemarker/templates as a default directly on CamelFreemarkerConfig.locations using @ConfigItem(defaultValue = "freemarker/templates")?

Could you please document the config param and the default in docs/modules/ROOT/pages/extensions/freemarker.adoc?

Copy link
Author

Choose a reason for hiding this comment

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

I've added the default location freemarker/templates to CamelFreemarkerConfig and also added some docs in docs/modules/ROOT/pages/extensions/freemarker.adoc

extensions/freemarker/runtime/pom.xml Outdated Show resolved Hide resolved
#
# Quarkus
#
quarkus.camel.freemarker.locations = org/apache/camel/component/freemarker
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that org/apache/camel/component/freemarker will be scanned recursivelly, I wonder whether all the class files are going to get included as native image resources? I would be interesting to compare the size of the native image using this locations value and using a new path that does not contain any class files.

Copy link
Author

Choose a reason for hiding this comment

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

Only files with the extension .ftl will be included. You can see there is a filter for that here... I've also created 2 native images, the first one using org/apache/camel/component/freemarker and the second one using myfreemarkerfolder and the sizes are exactly the same (45.8 MB)

Copy link
Contributor

Choose a reason for hiding this comment

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

Only files with the extension .ftl will be included.

Thanks for the info, I have not noticed that and I must say I do not think we should be selecting only *.ftl files. I commonly used other or no extensions for FreeMarker template files in the past. I think many people do that because because of syntax highlighting in editors. There is even a couple of examples in this repo: https://github.com/apache/camel-quarkus/tree/master/tooling/create-extension-templates

I think we should either include whole subtrees or we should have path includes and excludes like we have in https://github.com/apache/camel-quarkus/blob/master/extensions/core/runtime/src/main/java/org/apache/camel/quarkus/core/CamelConfig.java#L74-L100 WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

Good point @ppalaga . Files with different extensions should still work. I agree and we should include the whole subtree but just to avoid adding files we don't want to add we can use includePatterns and excludePatterns as you suggested. Let me add it

Copy link
Author

Choose a reason for hiding this comment

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

@ppalaga sorry for being so slow in this. I've added includePatterns and excludePatterns

@ppalaga
Copy link
Contributor

ppalaga commented Mar 3, 2020

Could you please rebase and squash your commits? It would make it easier to review locally.

@carlosthe19916
Copy link
Author

@ppalaga I've also added a new Config property called classModels which should be used to pass all Java Classes that will be marked for reflection. Freemarker uses POJOs for processing the templates but if those POJOs were not marked for reflection using Quarkus, then an exception will happen... Because of this change, I've removed this line 0e2d854#diff-d5f7db497629630202e093f916c76b9eL65 WDYT?

Copy link
Contributor

@ppalaga ppalaga left a comment

Choose a reason for hiding this comment

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

Some comments inline.

|===
|Configuration property |Type |Default

|`quarkus.camel.freemarker.locations`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we perhaps remove the locations option now that we have includes and excludes? The includes option seems to offer a superset of locations functionality. The default can be changed to freemarker/templates/** and moved to the includes option too. WDYT?


|`quarkus.camel.freemarker.include-patterns`

Used for inclusive filtering scanning of Freemarker templates. By default all tree files inside 'locations' will be included.
Copy link
Contributor

Choose a reason for hiding this comment

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

Plase add:

  • Where we scan for the templates (classpath, filesystem)
  • Are classpath: and filesystem: prefixes supported? Which one is the default? (I do not insist on supporting filesystem:) We have other extensions documenting similar features properly, just grep for classpath: in the source tree.

@ppalaga
Copy link
Contributor

ppalaga commented Mar 12, 2020

It would be cool if the extension could do most/all of the following tasks at build time:

  • Create a freemarker.template.Configuration
  • Parse/warmup the templates.

That would make it truly quarkyfied!

ppalaga added a commit to ppalaga/camel-quarkus that referenced this pull request Nov 25, 2020
ppalaga added a commit to ppalaga/camel-quarkus that referenced this pull request Nov 26, 2020
@ppalaga
Copy link
Contributor

ppalaga commented Nov 26, 2020

The tests were adapted in #2028 and I'd like to implement the native support using the Quarkiverse Freemarker extension. Hence closing this one.

@ppalaga ppalaga closed this Nov 26, 2020
ppalaga added a commit that referenced this pull request Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants