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

Introduce factories to create data group elements #16

Merged
merged 6 commits into from
Feb 2, 2019
Merged

Conversation

sprain
Copy link
Owner

@sprain sprain commented Feb 1, 2019

  • Use factories instead of setters in order to guarantee complete data group elements

Copy link

@ppietak ppietak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see changes we talked about :)
Do we still need nullable fields in Address? Like building number? Maybe finally it will be merged with different field either way?

src/DataGroup/Element/AdditionalInformation.php Outdated Show resolved Hide resolved
src/DataGroup/Element/AlternativeScheme.php Outdated Show resolved Hide resolved
src/DataGroup/Element/CombinedAddress.php Show resolved Hide resolved
@sprain
Copy link
Owner Author

sprain commented Feb 1, 2019

Do we still need nullable fields in Address? Like building number? Maybe finally it will be merged with different field either way?

IMO yes.
Not every address has a street.
Not every address with a street has a building number.

The purpose of StructuredAddress is to keep all these fields separate. This is how it's defined in the specs of the qr bill.

@sprain
Copy link
Owner Author

sprain commented Feb 2, 2019

I'll merge this in order to make users aware of the upcoming major refactorings. Further improvements may still be done.

@sprain sprain merged commit d4daf1a into master Feb 2, 2019
@sprain sprain deleted the factories branch February 2, 2019 07:37
@M4SX5 M4SX5 mentioned this pull request Sep 15, 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.

2 participants