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

Fix incorrect types #438

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

bensontrent
Copy link
Contributor

I made zip optional in IAddressCreateParameters since a zip code is not required in many countries. Also added optional service and tax_identifiers to IShipmentCreateParameteres to enable one-call buys.

A zip code is not required by some countries, for example, Antarctica.
Add missing options for ICreateShipmentParameters
@bensontrent bensontrent requested a review from a team as a code owner March 16, 2024 20:52
Copy link
Member

@Justintime50 Justintime50 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this, we'll need a couple changes to be able to roll this in:

  1. Although zip codes are not required for some countries, our API does expect zip codes presently for addresses. You will want to work with our support team to determine what may need to go in the zip code field for countries that don't support it. As such, we cannot accept the nulling of the zip fields on the address object as it would not be a correct change to make.
  2. With TaxIdentifier being a top-level object of our API, it would be better to place the type inside of its own folder structure like the other type objects are and follow the convention for defining it (with documenting the params as well like other objects).

If you'd rather not make these adjustments, let us know and we can continue the work you've started. Service and tax identifiers will be great additions that were previously missed.

@bensontrent
Copy link
Contributor Author

You can close this pull release, I'm just offering it as a suggestion.

Both zip and country are optional in IAddressCreateParameters according to the actual way the API works: the API will correctly return an error but the API does technically accept a blank value. The reason a zip ought to be optional: consider a new user-facing address form, all of those values would initially be blank. If my form object has a typescript type of IAddressCreateParameters, I'd have errors in my code since a zip code would initially be blank. Also we, know a street1 and a name are required to create a shipment, but having those values as optional seem to be correct way to handle the creation of a form object while the form is in the process of being filled out.

The changes I submitted were necessary for my typescript to validate, so I'll just use my own fork of the project to facilitate the needs of my project.

Thanks for looking at this!

@nwithan8 nwithan8 marked this pull request as draft March 20, 2024 21:36
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.

None yet

2 participants