faizel commented on pull request #421: URL: https://github.com/apache/cayenne/pull/421#issuecomment-625968759
@stariy95 I am able to provide an explicit option for specifying an external tool via `CgenConfiguration`, however in order to make it usable in 'ClassGenerationAction' without using any system properties, I have to defer the creation of the velocity context until it is needed because the needed configuration option is not available to `ClassGenerationAction` in time. It is needed in the constructor, however the configuration is not supplied until after the constructor is invoked (in `DefaultClassGenerationActionFactory`). This means I have to delay the velocity context creation in `ClassGenerationAction` until the `resetContextForArtifact(Artifact)` method is called. It works, but it obviously introduces a risk. There are a couple of alternatives to this approach, but both have trade offs: 1. Add `CgenConfiguration` as a parameter to `ClassGenerationAction`’s constructor and alter the `DefaultClassGenerationActionFactory` to supply the configuration during construction. That would allow the velocity context to be created where it currently is, in `ClassGenerationAction`’s constructor. This results in an API change for `ClassGenerationAction`'s constructor, however the constructor is only called from the `DefaultClassGenerationActionFactory` (the `CgenConfiguration` is already passed in to its `createAction` factory method as an argument). It would be a simple change to instantiate the action with the cgen configuration as argument rather than setting it immediately afterwards. Regardless, this may not be a desirable approach due to API change. 2. The other alternative is to leave all the code in `ClassGenerationAction` unchanged, and instead explicitly set **only** the `org.apache.velocity.tools` system property in the maven plugin if the `externalToolConfig` configuration is set. Obviously, this means that it would have to set a system property, however the upside is that it is limited in scope to the maven plugin and only one specific system property is set. The existing cgen code is unchanged (with the exception of CgenConfiguration of course). I would appreciate your thoughts on this as I don’t want to commit something that goes against your vision for Cayenne. My personal preference is for alternative 2, however I understand that is an approach you were trying to avoid. After that, I would prefer the scheme of lazily creating the context. If none of the three options is suitable, then I have no problem shelving this and maintaining something locally in my own project. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
