mark order shipping address optional

[nstapelbroek]

Description

Hi there,

It's been a long time coming (#271 ) but this PR changes the shipping address from value to pointer, allowing empty values to be skipped during serialisation.

I'm running my fork of the lib in the wild and it seems to work. Since it's been a while since I've contributed to a go lib repo I'm a bit skeptic that this is good enough to merge.

• What do you think about the test, feels a bit misplaced tbh
• Changing the method signature breaks the API design, so it's backwards incompatible with v3 right?
• Running tests on go 1.20 breaks, is that just me?

Let me know what you think ;)

Motivation and context

When selling digital goods there is no need to send a shipping address when creating an order. The API docs even dictate that this address is optional. I merely made the field optional in the api client library.

See #271 for our initial discussion.

How has this been tested?

Did a test order on my local app running a mollie integration in test-mode.
You can see that the order no longer has the address attached to it. Here's a before and after:

• Unit tests added / updated
• The tests run on docker (using make test)
• The required test data has been added / updated

If you are running the tests locally, please specify:

• OS: macos
• Go version 1.21

Types of changes

What types of changes does your code introduce? Put an x in all the boxes that apply:

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• This change requires a documentation update

Checklist

Go over all the following points, and put an x in all the boxes that apply.

Please, please, please, don't send your pull request until all of the boxes are ticked. Once your pull request is created, it will trigger a build on our continuous integration server to make sure your tests and code style pass.

• I have read the CONTRIBUTING document.
• My pull request addresses exactly one patch/feature.
• I have created a branch for this patch/feature.
• Each individual commit in the pull request is meaningful.
• If my change requires a change to the documentation, I have updated it accordingly.

[VictorAvelar]

Hey! Thanks for the follow up PR, I am going to have a look at this today and then I will get back to yo. 🚀

[VictorAvelar]

Hey @nstapelbroek,

I've reviewed your pull request, and overall, it looks good. Thanks for making those changes.

Regarding your questions:

What do you think about the test; it feels a bit misplaced, to be honest?

The test is okay from a coding perspective, but it doesn't seem to cover what your changes are supposed to address. From my perspective, I would suggest testing that the serialization from JSON to the struct, when no OrderAddress is provided, results in nil. Conversely, when no OrderAddress is provided, the JSON should not contain the related key-value pair.

Changing the method signature breaks the API design, so it's backwards incompatible with v3, right?

I'm already working on some breaking changes, so the next tag will likely be v4. For v3, I can either leave it as is or add a note to the readme.

Running tests on go 1.20 breaks; is that just me?

The CI runs tests from go 1.18 up to 1.20, and all of them are passing.

If you'd like to add the described tests, that would be fantastic. Otherwise, let me know, and I can take care of them. I'm planning to run a check on all structs to ensure there are no similar issues in the library for v4.

[VictorAvelar]

Superseded by #327