Skip to content

Conversation

@johnjaylward
Copy link
Contributor

@johnjaylward johnjaylward commented Jul 21, 2020

Fixes #541

What problem does this code solve?

Refactors the XML Configuration to use a builder pattern instead of public constructors with many parameters. This makes viewing the configuration easier to read and maintain.

Risks

The fields on the configuration have changed visibility from public to private and are now accessible via public getters. This means that a new release will not be a drop-in replacement and consumers of this library will need to correct code that is directly referencing those fields. This should hopefully be minimal impact as the fields were readonly (i.e. hopefully no one was actually using them).

Changes to the API?

Yes, public fields were marked private and constructors were deprecated. New methods were put in place for managing the XML Configuration.

Will this require a new release?

Yes

Should the documentation be updated?

I don't believe we have any documentation outside of the Java-docs referencing this API.

Does it break the unit tests?

No, but unit tests were updated to use the new API.

Was any code refactored in this commit?

Yes, as mentioned above.

Review status

APPROVED

John J. Aylward added 2 commits July 21, 2020 11:08
Updates XML configuration to use a builder pattern instead of
constructors with many parameters
@stleary
Copy link
Owner

stleary commented Jul 21, 2020

Starting 3-day comment window.

@johnjaylward
Copy link
Contributor Author

johnjaylward commented Jul 23, 2020

@stleary, would you mind using tags/labels to make it easier to see which PR are up for review and which are ready to commit? Not that we usually have a lot, but it may make it easier to see at a glance from the main PR screen.

@stleary stleary merged commit 7852810 into stleary:master Jul 24, 2020
This was referenced Mar 10, 2021
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.

XMLParserConfiguration needs to be opened up.

2 participants