Add support for customs information on a parcel #4
Conversation
|
|
||
| // Weight contains the weight in Kilograms. Supports up to 3 decimals | ||
| // (grams). For example: decimal.New(20, -3) specifies 20 grams. | ||
| Weight decimal.Decimal |
There was a problem hiding this comment.
This incurs a breaking change in the API of this package. The Weight field was typed as int64. I don't like breaking changes so initially tried to make it backwards compatible but found that it was impossible to do that because previous behavior was unspecified.
The Weight field here was not sent to SendCloud. It was just ignored. There is no documentation on the field that specifies whether it is expected to have the weight in Kilograms, grams, pounds, whatever. So if we were to silently start sending this value to SendCloud it might cause issues with existing users of the package that use a different unit from whatever we choose to convert/send (e.g. they set it in grammes, we send it in Kilos).
Therefore I propose to explicitly break existing users of this package (if they set the Weight field), forcing them to validate their code and explicitly pass a decimal.Decimal value in Kilograms.
There was a problem hiding this comment.
Good catch about the in64! We'd like to keep the dependencies of this package to a minimum, so I don't think using an external package to do something a float64 could do is what we're looking for.
You're right that there's no documentation in this package about the type of weight we're expecting. However, the Sendcloud documentation specifies that they need the weight in kilograms (https://docs.sendcloud.sc/api/v2/shipping/#create-a-parcel). It's a great idea to document it in this package like you did here.
Keeping in mind that Sendcloud does specify they want the weight in kilograms, it would be a mistake on the consumer side to send grams in the first place.
Let me know if you have a case against using float64 types rather than decimal.Decimal in this case.
There was a problem hiding this comment.
I agree that external dependencies is something to avoid. In this case I needed it because the Sendcloud API places a constraint on the number of decimal places that can be sent. Otherwise it returns this error:
2021/08/03 17:01:02 failed to create a new parcel at sendcloud: request api/v2/parcels resulted in error code 400: weight: "Ensure that there are no more than 3 decimal places."
Also, a float is not an accurate decimal type. It shouldn't be used for things like weight and money/value.
So I would still suggest to include the github.com/shopspring/decimal package as a dependency, it is a popular and well-maintained package.
GeertJohan
force-pushed
the
feature/international-parcel-support
branch
from
727cc58 to
e13fcfe
Compare
| Weight decimal.Decimal | ||
|
|
||
| CustomsInvoiceNr string | ||
| CustomsShipmentType int64 |
There was a problem hiding this comment.
Maybe this package should define the possible values as a typed const (enum-esque construction)?
There was a problem hiding this comment.
I agree, good idea!
|
|
||
| // Weight contains the weight in Kilograms. Supports up to 3 decimals | ||
| // (grams). For example: decimal.New(20, -3) specifies 20 grams. | ||
| Weight decimal.Decimal |
There was a problem hiding this comment.
Good catch about the in64! We'd like to keep the dependencies of this package to a minimum, so I don't think using an external package to do something a float64 could do is what we're looking for.
You're right that there's no documentation in this package about the type of weight we're expecting. However, the Sendcloud documentation specifies that they need the weight in kilograms (https://docs.sendcloud.sc/api/v2/shipping/#create-a-parcel). It's a great idea to document it in this package like you did here.
Keeping in mind that Sendcloud does specify they want the weight in kilograms, it would be a mistake on the consumer side to send grams in the first place.
Let me know if you have a case against using float64 types rather than decimal.Decimal in this case.
| Weight decimal.Decimal | ||
|
|
||
| CustomsInvoiceNr string | ||
| CustomsShipmentType int64 |
There was a problem hiding this comment.
I agree, good idea!
| type ParcelParamsItem struct { | ||
| Description string | ||
| Quantity uint64 | ||
| Weight decimal.Decimal |
There was a problem hiding this comment.
These could be float64
There was a problem hiding this comment.
Hi @roelofjan-elsinga , sorry for the late reply!
I tried float64 but some API calls failed because four decimal places are not supported (8.1234 kg). This is very easy to occur with floats64's, as they don't enforce the number of decimal-places. The shopsprig/decimal package is very popular, imported by over 30k public Go packages (and probably even more private ones) it's the de-factor decimal package for Golang. I don't think users of the sendcloud package would mind having it as indirect dependency.
|
Better late than never @GeertJohan: let's merge this! |
Let's do! I'll rebase this PR shortly. 😃 |
|
@roelofjan-elsinga While rebasing I found the upstream has added a number of fields that are also in this PR. Some of the fields are now a string, for example |
|
@GeertJohan Yes, let's keep it consistent 👍 |
No description provided.