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 'customRuleFactory' config option for pluggable RuleFactory #211

Merged
merged 1 commit into from
Aug 4, 2014

Conversation

davidpadbury
Copy link
Contributor

As discussed in #210, this request adds supports for specifying a custom RuleFactory enabling consumers to hook in custom rules to modify generation behavior.

Please consider this initial request only an invitation to opine on the approach. Although the implemented behavior and IT successfully demonstrates the extension is possible, I'm rather horrified to have actually used a FactoryFactory.

The problem resides with that current a RuleFactory implementation depends on being passed the config, annotator, and schema store. Rather than invoke a particular constructor via reflection that feels awfully messy, I opted for creating something to have an interface with those dependencies and returning a RuleFactory. However we now have our FactoryFactory. I considered naming the thing a "creator" or something, but that seems to skirt around the issue.

What do you think? Is this exposing a design issue, or should we just name something less embarrassing and move on?

@joelittlejohn
Copy link
Owner

Hi David. Thanks for putting this together. Apologies for the delay, I've been on vacation.

I see the problem you have with this 'FactoryFactory'. My first thought was just to call this a RuleFactoryCreator then I re-read your comment above and saw that you suggested exactly the same thing :) I also thought we could take some inspiration from the strategy pattern and think of this thing as the 'RuleFactoryContext'.

I agree the FactoryFactory is a rather embarrassing type to create, but leaving the name aside for a moment I also don't like that introducing this type makes it harder for people to understand how to use this new feature you've added.

I can think of two options to push these dependencies into the RuleFactory interface so that they are made explicit:

  • An init method like:
void init(GenerationConfig generationConfig, Annotator annotator, SchemaStore schemaStore);
  • Setters, like:
void setGenerationConfig(GenerationConfig generationConfig);
void setAnnotator (Annotator annotator);
void setSchemaStore(SchemaStore schemaStore);

The latter would allow easier additions.

Obviously there's no way of indicating in the type definition that these will be called before the RuleFactory is used, this would need to be done in the javadocs. However I think these are very easy to understand when people want to implement their own RuleFactory.

What do you think?

@joelittlejohn joelittlejohn added this to the 0.4.5 milestone Jul 7, 2014
@joelittlejohn
Copy link
Owner

@davidpadbury Hi, are you still interested in including this in 0.4.5?

@davidpadbury
Copy link
Contributor Author

Yes. Sorry. Been rather side tracked by real work. Should get to shortly.

I expect to follow your recommendation. Likely with setters against an
interface so for those who do want to extend the capabilities provided are
clear.

On Fri, Jul 25, 2014 at 9:02 AM, Joe Littlejohn [email protected]
wrote:

@davidpadbury https://github.com/davidpadbury Hi, are you still
interested in including this in 0.4.5?


Reply to this email directly or view it on GitHub
#211 (comment)
.

@davidpadbury
Copy link
Contributor Author

Just pushed in an update to use custom instances of RuleFactory rather than a separate factory. Actually makes implementation of custom RuleFactories far prettier.

What do you think?

@joelittlejohn
Copy link
Owner

This looks great. Could you squash all these commits? I'll merge for
inclusion in 0.4.5.

Thanks again for contributing. I think a lot of people will find this
useful.
On 3 Aug 2014 21:33, "David Padbury" [email protected] wrote:

Just pushed in an update to use custom instances of RuleFactory rather
than a separate factory. Actually makes implementation of custom
RuleFactories far prettier
https://github.com/davidpadbury/jsonschema2pojo/blob/master/jsonschema2pojo-integration-tests/src/test/java/org/jsonschema2pojo/integration/config/CustomRuleFactoryIT.java#L56
.

What do you think?


Reply to this email directly or view it on GitHub
#211 (comment)
.

@davidpadbury
Copy link
Contributor Author

Has now been squashed. Any rough idea when you'll cut 0.4.5?

Glad to have helped. The design of jsonschema2pojo seems very well done, a pleasure contributing too.

@joelittlejohn
Copy link
Owner

Thanks, glad this is the case.

I have no need to hold up 0.4.5, I'll release soon.

joelittlejohn added a commit that referenced this pull request Aug 4, 2014
@joelittlejohn joelittlejohn merged commit 53cb922 into joelittlejohn:master Aug 4, 2014
@joelittlejohn joelittlejohn changed the title Pluggable RuleFactory Add 'customRuleFactory' config option for pluggable RuleFactory Aug 4, 2014
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.

2 participants