Skip to content

Conversation

@jchen293
Copy link
Contributor

@jchen293 jchen293 commented Jul 11, 2024

Description

Change desired_delivery_date to str instead of key-word args in Shipment.recommend_ship_date function

Testing

We should expect users to input a string and not a dict for the Shipment.recommend_ship_date function, this matches retrieve_estimated_delivery_date function

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)

@jchen293 jchen293 requested a review from a team July 11, 2024 17:37
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.

Are we doing this for all the libs? What if a param gets added?

@jchen293
Copy link
Contributor Author

Are we doing this for all the libs? What if a param gets added?

We haven't implemented this in the other six libraries yet, so we can make a decision here. This change matches retrieve_estimated_delivery_date above, which expects a string, an inconsistency I just discovered. The precision_shipping endpoint expects desired_delivery_date which should always be a single date parameter, so I don't foresee the API making a breaking change. Since both shipment smartrate endpoints are somewhat similar, this is more of a question of whether we want to wrap the parameter for users in the client libs.

@jchen293 jchen293 merged commit 05e9869 into master Jul 11, 2024
@jchen293 jchen293 deleted the fix_recommend_ship_date_parameter branch July 11, 2024 18: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.

3 participants