-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Templating Engine API and handlebars support #690
Conversation
this sounds good, backwards compatibility is always a very nice thing ;-) |
I don't know if "migration to handlebars" is a great thing. I'd rather see a pluggable templating engine that each framework can choose either a built-in implementation (we could support mustache + handlebars) or a community-driven adapter for some other engine. One thing I'm a little cautious about with this PR is that it allows users to mix mustache and handlebars templates within the same generator. We can guard against this in the generators in the repository, but we'd have no real control over what custom templates users implement. |
@jimschubert I'm not sure what would be the problem in mix and matching templating engines in a given generator. For generators that have a large template codebase mix and matching would alleviate the effort needed to fully migrate to a different template. Or use a certain templating engine because it's simpler for some files, but too heavy for other files ? I agree though, a pluggable system would be great 👍, registering a given plugin for a given file extension ? Or a system where the templating engine is selected by a command line option ? |
d97f7fb
to
450a6d1
Compare
@rienafairefr my concern with mixing two template types is that users will begin customizing on this, and it will become more difficult to "migrate" from one as default to another (that is, it'll more likely always be mixed). I have plans to eventually create an interface for transforming files, and provide both a Mustache and a Handlebars implementation. Types implementing For instance, the Ruby community may prefer ERB. The JavaScript community may prefer haml or something else. It would be great if one could do:
This should remove one of the larger barriers I've seen users have, which is modifying the mustache templates and trying to figure out the object structure passed into the templates. I have an image in #503 which I think shows the extraction of a template engine a bit. I'd also like to have a clearly defined type being passed into the engine of choice rather than the current I consider template engine cleanup to be part of the medium-term efforts defined on the roadmap. I'll try to find some time in the next few weeks to create a tracking project for these types of design considerations. |
@jimschubert OK I see your point, would be better as you described it in #503 among others. Reworking this PR's code to have an interface for templating engines is something I could maybe do, might send an update. For the usage,
is modifying the classpath the only way ? I could see using a plugin framework for that, a subfolder 'plugins' in the working directory would contain JARs, each implementing a tempalting engine, then the A fixed type for the template engine input seems also good to me, I'm also quite allergic to |
@rienafairefr I've done a prototype that has a plugin system. A (minor?) issue with doing the plugin route is that it could cause versioning conflicts if the plugins pulled in dependencies we're already using. You could get that with the class path usage, but this way wouldn't be hidden from the user. If you do get a chance to create a template engine interface, that would be fantastic. If not, I'm going to try to get it done in the next week or two. |
450a6d1
to
72f0b4b
Compare
OK thanks for the pointers. I have something working for templating interface (and a branch built on it with plugins), I'm pushing it to this PR, should be simpler than a new one and will keep these conversations. |
3b837f1
to
5365210
Compare
5365210
to
0c65ea2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments. I see that you're actively committing, so my review isn't 100% complete. I just wanted to submit comments on what exists now.
</parent> | ||
<modelVersion>4.0.0</modelVersion> | ||
|
||
<artifactId>openapi-generator-api</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this package be renamed to openapi-generator-core
? The -api
seems redundant with the openapi
, and some may confuse this package with openapi-generator-online
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, that's a better name for sure.
/* | ||
Used to determine whether a given supporting file is a template | ||
*/ | ||
String getFileExtension(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be an array. For example, Handlebars common extensions are .hbs
and .handlebars
.
.compile(template); | ||
|
||
writeToFile(outputFilename, tmpl.execute(bundle)); | ||
if (templateFile.endsWith(templatingEngine.getFileExtension())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A key point that is missing here is:
compiler = config.processCompiler(compiler);
This allows generators to extend or configure the Mustache.Compiler
instance. See example at
Line 146 in ab9c4b5
public Mustache.Compiler processCompiler(Mustache.Compiler compiler) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see.
These are changing some Mustache-specific parameters though, not sure I I extract those to the TemplatingEngineAdapter interface, or I can just do the compiler = config.processCompiler(compiler);
step only when using a MustacheEngineAdapter ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, 2nd option is definitely the easiest to implement, just pushed that. I think I'll be adding a processTemplatingEngine
to the CodegenConfig
interface, and *Codegen writers could do it themselves, casting to their particular language/generator preferred TemplatingEngineAdpater
public interface TemplatingGenerator { | ||
|
||
// returns the template content by name | ||
String getFullTemplate(String name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name isn't too clear to me (is it a full path to the template, or template contents)? The comment clarifies this, and should be a standard JavaDoc comment.
Could this be renamed to getFullTemplateContents
for clarity? Usually, naming isn't that important, but this will quickly become a method name required by implementers which makes it harder to change later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, getFullTemplateContents seems better to me also :-)
|
||
writeToFile(adjustedOutputFilename, tmpl.execute(templateData)); | ||
String templateContent = templatingEngine.doProcessTemplateToFile(this, templateData, templateName); | ||
writeToFile(adjustedOutputFilename, templateContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, also missing extension of the templating engine:
compiler = config.processCompiler(compiler);
@rienafairefr I forgot to include this in my review comments, but the new module will need to be added to the root Dockerfile as well. diff --git a/Dockerfile b/Dockerfile
index e53ca77d8..24e0b1814 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -18,6 +18,7 @@ COPY ./modules/openapi-generator-gradle-plugin ${GEN_DIR}/modules/openapi-genera
COPY ./modules/openapi-generator-maven-plugin ${GEN_DIR}/modules/openapi-generator-maven-plugin
COPY ./modules/openapi-generator-online ${GEN_DIR}/modules/openapi-generator-online
COPY ./modules/openapi-generator-cli ${GEN_DIR}/modules/openapi-generator-cli
+COPY ./modules/openapi-generator-api ${GEN_DIR}/modules/openapi-generator-api
COPY ./modules/openapi-generator ${GEN_DIR}/modules/openapi-generator
COPY ./pom.xml ${GEN_DIR}
|
OK, I've added the module to the dockerfile, thanks |
38c1fdc
to
7fa582c
Compare
OK @jimschubert I think I converged on something working with your review, thanks. Anyone else I should @mention ? Should this still target master, BTW ? Creating the -core package interface seems to me like a major change. |
@rienafairefr this should target the 4.0.0 branch as it is breaking changes (changing the CodegenConfig interface). |
6921769
to
0ee5694
Compare
…gen classes if needed
Hey @rienafairefr, @jimschubert, and @wing328 Can you please make sure I'm not going crazy? I am still not able to generate the correct api spec when using the The issue is that I have this command
And I'm doing a very simple template configuration. There are three templates I'm providing for generation. The three files are
The template files look like this
model.hbs
partial_header.hbs
However the output I'm getting is looking like this
and the api information looks entirely different from the template as well. HOWEVER, it looks EXACTLY like the default template provided by the open api generator's
Steps to Reproduce
Conclusion Super keen to hear your feedback on this folks! As always keep up the great work! All of your hard work is very much appreciated! |
5233a61
to
96dbb43
Compare
@wing328 just rebased on latest master, including the modifications by @jimschubert (Thanks!) |
@rienafairefr I asked a couple of cohorts and the compilation issue appears to be machine specific. However the Is there any chance you can provide an example of how to do code generation using the |
@loganknecht I've spent a little time evaluating the example you presented above, I really appreciate it. It looks like the branch only supported the default Java Bean style accessors, so if you were trying to access any properties in the bundle we pass to the template which don't follow Java Bean style… you wouldn't have been able to access those properties. This is likely the problem you're seeing. I pushed a branch to this repo at https://github.com/OpenAPITools/openapi-generator/tree/rienafairefr-templating. I was wondering if you could try this out? If it works, we can go ahead and merge that branch which is based off of this PR's head commit as it also updates README.md with the new option and a caveat that only Mustache is officially supported for embedded templates. Here's a minimal api.hbs to get you started in evaluation on that branch:
Running this locally appears to generate expected struct and signatures (I cleaned some whitespace manually):
|
Hey @jimschubert! Sorry to leave you hanging on validating this! I did try to validate this, but I keep having personal system errors with some type of locale issue. For clarification, my development system is set to Japanese, and I'm not sure if that's causing an issue. What happens is I get test failures for something called a "Forbidden API usage" when string.format is called in the Because of this I don't know if I will be able to correctly verify this. If you think this is ready and that it can be used in conjunction with the Thank you so much for all the great work you guys are doing! |
@loganknecht thanks for the information. Sounds like a static analysis error. I'll rerun later today or tomorrow and try to repro the error you're seeing. I may have only done a "quick build" which skips analysis, thinking it only skips unit tests. |
Checking in: I haven't yet had time to evaluate the language issue from above. I haven't forgotten about it. |
@loganknecht I see. I don't evaluate changes using Here's a diff you can apply locally: diff --git a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/api/AbstractTemplatingEngineAdapter.java b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/api/AbstractTemplatingEngineAdapter.java
index 19965f2e6d..3255b9c858 100644
--- a/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/api/AbstractTemplatingEngineAdapter.java
+++ b/modules/openapi-generator-core/src/main/java/org/openapitools/codegen/api/AbstractTemplatingEngineAdapter.java
@@ -1,5 +1,7 @@
package org.openapitools.codegen.api;
+import java.util.Locale;
+
/**
* Provides abstractions around the template engine adapter interface, for reuse by implementers.
*/
@@ -17,7 +19,7 @@ public abstract class AbstractTemplatingEngineAdapter implements TemplatingEngin
String[] result = new String[extensions.length];
for (int i = 0; i < extensions.length; i++) {
String extension = extensions[i];
- result[i] = String.format("%s.%s", getPathWithoutExtension(location), extension);
+ result[i] = String.format(Locale.ROOT, "%s.%s", getPathWithoutExtension(location), extension);
}
return result;
} I've also pushed the change to the previously mentioned branch. Let me know if that works for you, and I'll coordinate with the core team about next steps for the PR. |
@rienafairefr @loganknecht Hope you can help me. I tried to use your fork but couldn't get it to work: The folder
|
Got it working from branch rienafairefr-templating by manually creating the missing files as copies from the mustache templates. Afterwards I realized, there is no possibility to add handlebars helpers, thus limiting the power of handlebars. Additionally, this concerns me: Line 43 in f95715f
It's basically an empty try-catch and I'd rather be informed about invalid templates rather than errors being silently swallowed. |
@mnboos This branch is an initial implementation to get such feedback, and your initial take is very helpful. I agree about the missing helper issue, maybe we could support skipping this via some property ("strict-templating"?). We tend toward making things easier for new template/generator authors, so I'd like to avoid failing too hard if someone is coming from mustache to handlebars... but we should log a warning at the very least when this occurs. The intent of the templating abstraction is to allow generator authors to extend templating to support their needs. Ideally, I'd like to support a small set of helpers with the built-in implementation. Do you have suggestions about which helpers are the most useful? |
@jimschubert Thank you for your response! Just logging any missing handlebars helpers is totally fine, especially in terms of robustness. I was missing a helper like Additionally, I'd like to have a way to create own templates. As far as I know, if I create a completely new template, it won't be processed. This would be helpful, for example for creating a custom test class or something else. What is also a bit unclear to me is whether I have to have all templates available for the chosen templating engine or if it should fall back to mustache. |
I've created #2656 to track user-defined extensions to the templates file arrays which the generator processes. I'll look at what it would take to define some built-in helpers similar to what we have for mustache. |
I've opened #2657 with a clean merge of master, planning to get this merged before the 4.0 release. Closing this in favor of 2657. |
PR checklist
master
,4.0.x
. Default:master
.@wing328 @jimschubert
Description of the PR
After reading the discussion #510 about switching template systems I thought why not try to implement Handlebars support
In order to suport migration to handlebars templates, I've added the handlebars.java, and in the Generator if the file ends with .mustache then we use Mustache, if it ends with .handlebars we use Handlebars. Keeps backward compatibility easily.
There is a small compatibility fix for maximum compatibility with Mustache (see here
No templates for any language have been migrated for now.