cayenne-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [cayenne] faizel commented on pull request #421: Set system properties in pom.xml for maven plugin's cgen goal
Date Fri, 08 May 2020 19:03:18 GMT

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:
users@infra.apache.org



Mime
View raw message