Webador / sendcloud

Provides a PHP client to interact with the SendCloud API in an object-oriented way.
MIT License
16 stars 15 forks source link

Add default value for houseNumber. #24

Closed lukas-jansen closed 1 year ago

lukas-jansen commented 1 year ago

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

villermen commented 1 year ago

@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:

lukas-jansen commented 1 year ago

@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 commented 1 year ago

@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: https://github.com/Webador/sendcloud/pull/24/commits/1bb491049c92b5dc4009dc94bc10d6b2605b1a8b#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 commented 1 year ago

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 commented 1 year ago

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