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

[feat] Add missing options #593

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

[feat] Add missing options #593

wants to merge 2 commits into from

Conversation

nwithan8
Copy link
Member

@nwithan8 nwithan8 commented Oct 4, 2024

Description

Add all missing shipping options.

NOTE: Some options are/continue to be the wrong type (e.g. string instead of bool, string instead of DateTime (#520)). This PR is purely additive, with only new options and documentation on existing options to point out where a type is invalid. We should consider fixing these in a breaking change in the future.

Closes #592

Testing

Pull Request Type

Please select the option(s) that are relevant to this PR.

  • 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 not work as expected)
  • Improvement (fixing a typo, updating readme, renaming a variable name, etc)

@nwithan8 nwithan8 marked this pull request as ready for review October 8, 2024 20:14
@nwithan8 nwithan8 requested a review from a team as a code owner October 8, 2024 20:14
Copy link
Member

@jchen293 jchen293 left a comment

Choose a reason for hiding this comment

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

Why do some options have a type comment while others don't?

[JsonProperty("carrier_insurance_amount")]
public string? CarrierInsuranceAmount { get; set; }
public string? CarrierInsuranceAmount { get; set; } // number
Copy link
Member

Choose a reason for hiding this comment

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

Should the comment be any?

Suggested change
public string? CarrierInsuranceAmount { get; set; } // number
public string? CarrierInsuranceAmount { get; set; } // any

@nwithan8
Copy link
Member Author

Why do some options have a type comment while others don't?

Those with comments are to denote options that are technically the wrong type (e.g. a "string" type, but server-side, it parses it as a number. An end-user could potentially provide a non-numeric string, which would cause issues server-side). These should be considered for changes during the next breaking changes release cycle.

@Justintime50
Copy link
Member

As mentioned in the past, I don't believe exposing all available options is the right play. Many are intentionally undocumented for various reasons - many need to be vetted for validity. I would caution against moving forward with this PR and instead continue having users rely on the custom option override you built to allow passing options that may not be officially supported.

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

Add support for "merchant_id" shipment option
3 participants