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

SinceID as Zero and omitempty #89

Closed
luthermonson opened this issue Feb 24, 2020 · 5 comments
Closed

SinceID as Zero and omitempty #89

luthermonson opened this issue Feb 24, 2020 · 5 comments
Labels
4.0 Will address this in the next major release, either because it's a breaking change or major effort.

Comments

@luthermonson
Copy link

Problem
SinceId is an int64 but the API documentation tells you to pass since_id=0 to work with paginated results. I found a problem in the smartcollections end point where it sorts by alpha title on default so in order to force an id sort you have to to use since_id=0 properly and have to initialize that param to force the id sort. With omitempty on SinceID the marshaler will consider this empty and not send the param even if you explicitly set it to int64(0) since obviously omitempty considers zero an empty int64 type. Also, if you don't set the param the struct will still initialize with zero and you dont want to pass since_id=0 ALL the time so you can't just remove omitempty.

Workaround
Initialize SinceID as int64(1) as it is basically the same thing for all intents and purposes but is not technically not what the api documents as the proper method and could trip up a developer.

Fix?
Possibly just document as a limitation on the struct field

@oliver006
Copy link
Collaborator

For scenarios where you need control over including e.g. an int64 field in your json output but still need it to be zero, the best way I've seen is making the int a reference in the struct and then set it to nil if you want to omit it or = &zero (where zero := 0) if you want to include a zero value.

@luthermonson
Copy link
Author

Ya that is how I'd get around that too is add a nil option. Changing it now would break existing code so if this route was taken it should probably be included in a new major version

@oliver006
Copy link
Collaborator

🤷‍♀ - you could make the backwards incompatible change and release it as a new major version. Should be an easy change for callers as long as it provides a convenience function Get/Set Id() or similar that sets/returns 0 in case the struct value is nil.

@oliver006 oliver006 added the 4.0 Will address this in the next major release, either because it's a breaking change or major effort. label Jan 22, 2024
@oliver006
Copy link
Collaborator

Addressing this in #258 (by making SinceId a *uint64 )

oliver006 added a commit to oliver006/go-shopify that referenced this issue Jan 22, 2024
oliver006 added a commit to oliver006/go-shopify that referenced this issue Jan 22, 2024
oliver006 added a commit to oliver006/go-shopify that referenced this issue Jan 26, 2024
oliver006 added a commit to oliver006/go-shopify that referenced this issue Jan 26, 2024
oliver006 added a commit to oliver006/go-shopify that referenced this issue Jan 31, 2024
oliver006 added a commit to oliver006/go-shopify that referenced this issue Jan 31, 2024
oliver006 added a commit that referenced this issue Jan 31, 2024
* make AuthorizeUrl() return an error

* NewClient now returns an error, MustNewClient can be used if a panic is acceptable

* Address #89 - SinceID as Zero and omitempty

* fix variable names

* int64 --> uint64

* README.md
@oliver006
Copy link
Collaborator

v4.0.0 was released - this should be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.0 Will address this in the next major release, either because it's a breaking change or major effort.
Projects
None yet
Development

No branches or pull requests

2 participants