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 default value for houseNumber. #24

Merged
merged 5 commits into from
Jul 4, 2023

Conversation

lukas-jansen
Copy link
Contributor

@lukas-jansen lukas-jansen commented Jul 2, 2023

Changed street parameter naming to match sendcloud API naming
Closes #23

@villermen
Copy link
Member

@lukas-jansen Thanks for the contribution! I've looked through it and played around with it for a bit. I'm going to add some changes to address some concerns before this can be merged:

  • Street can no longer be obtained separately on obtained parcels. I can imagine that's still a valid use case for some.
  • It's not clear that leaving house number empty will auto-parse it (it will, I verified). null is not accepted by the API however. I think leaving out the nullable parameter entirely when it's null is easier to interpret.
  • Address and house number can be supplied together, working additively. I don't think that's a concern to fix, but I can add a docblock to make that clear.

@lukas-jansen
Copy link
Contributor Author

@villermen The street and housenumber can still be obtained separately https://github.com/Webador/sendcloud/pull/24/files#diff-0776c3ed4485bbdfe0e342af8a73172d8fe7c73ec79d485a529929c425a268b3L12-L13 Sendcloud will still return the address divided parameters with the street and housenumber it parsed from the address

@villermen
Copy link
Member

@lukas-jansen You're right! I meant to say I want to support both situations without having to do add or split the addresses ourselves. Like so: 1bb4910#diff-0776c3ed4485bbdfe0e342af8a73172d8fe7c73ec79d485a529929c425a268b3R7-R29

Question on the side: I know the API calls it "address", but we already call our model that way. Do you reckon calling the property "addressLine1" would make things more clear or more confusing?

@lukas-jansen
Copy link
Contributor Author

lukas-jansen commented Jul 3, 2023

In our case (Craft CMS) is using this package for addressing, what follows the https://github.com/commerceguys/addressing
There it's also called addressLine1 so in that way it would be clear

@villermen
Copy link
Member

@lukas-jansen Thanks for the solution and thinking with me on this! I'll push a major release that includes these changes shortly.

@villermen villermen merged commit 65529ad into Webador:master Jul 4, 2023
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.

House number is not required
2 participants