Features/builders as inner classes

[dewthefifth]

Adds support for using the https://en.wikipedia.org/wiki/Builder_pattern instead of just chainable setters. This commit maintains full backwards compatibility with existing capabilities, but enables the selection of a more robust builder pattern if desired (for example if you want to have immutable objects).

This pull request also includes multiple refactoring changes to avoid duplicate code wherever possible. Specifically, when implementing the builder mechanisms several capabilities which were already implemented in the code needed to be migrated into more accessible places.

[joelittlejohn]

Thanks for submitting this, it looks like a great change.

Could you explain why you introduced two new configuration properties to control this? I think we only need one: useInnerClassBuilders (default false).

[joelittlejohn]

I will take a look at why build is failing with the gradle download error.

[dewthefifth]

The second configuration option is to enable both the chainning setters and the inner builder to be enabled simultaneously. I can't think of why you would need such a thing, but I wanted to make sure I wasn't missing something.

I'm not actually sure how to put a PR on hold or I would.

It occurred to me last night that I haven't tested the builders using different constructor options and required fields. It's possible that the generated code will not compile if an inner builder is chosen and it's generating constructors for required fields. (The inner builder requires the use of an empty constructor right now). I'd planned to add the various constructors later, because I want to add in a copy constructor, but based on my thoughts last night I'm not sure I can wait without introducing issues. (Obviously all issues would be when using the new feature).

I'll get back to this later this week and add in the constructors, but I'll still end up doing the copy constructors as a different PR.

[dewthefifth]

Oh, and if you want me to pull out 'isChainableSettersBuilders' let me know and I will.

[joelittlejohn]

Yes, I think remove isChainableSettersBuilders. If you turn on builders using <generateBuilders>true, you should then use <innerClassBuilders>true if you want inner builder classes.

[dewthefifth]

I removed the isChainableSettersBuilder configuration, and added configuration options for innerBuilders to the cli, ant, grandle projects. I'm not actually familiar with ant/gradle/cli so please take extra care to look at those and make sure I didn't miss anything.

I also added code to correctly invoke non-empty instance constructors when creating the builders instance member, and added code to avoid duplicate instance initialization when inheriting a builder.

I noticed some strange inheritance behavior during my testing, but I think that it might be 'normal' behavior. Mainly, if the superType and the subType both declare the same property then it will be included in both classes instead of just the parent class. I don't think this is a problem I introduced, but something that was already there (or intended behavior?).

Future enhancements would include

• Additional integration tests confirming that the builders are being generated correctly

I'm not actually sure why the checks keep failing, because I've run the build multiple times locally and even done some testing against my own schemas using the maven plugin that gets built. Let me know if there's anything more you need from me on this.

[dewthefifth]

I looked into the travis build again, I thought it was failing on the gradle downloads but it looks like it had changed from gradle anger to ant anger. I tracked down the failing integration/unit test for ant and it's passing now. I'll check in again later to make sure everything is passing in the travis build.

[joelittlejohn]

Fantastic work! So it sounds like the last remaining thing here is some integration tests. We'll need at least some minimal integration tests before merging this, because I don't like having a feature that can disappear/break/regress if something changes.

Thanks for being so thorough with this PR though, I appreciate this is not an easy change and quite ambitious. Great feature though that I think will be useful for a lot of people.

[dewthefifth]

Integration tests have been added. Let me know how things look.

[joelittlejohn]

I've merged and will release in the next version. Thanks Duane, this is an excellent change implemented very carefully and well!