-
Notifications
You must be signed in to change notification settings - Fork 7
Adding Luma examples #205
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
Adding Luma examples #205
Conversation
Justintime50
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went halfway through the PR and found a few issues that need to be resolved.
Best way to verify these work as intended is to write them up (as you've done, nice work - I know it can be a lot!) and then actually run them to ensure the response we got back made sense and there weren't any syntax errors or missing params. To go along with our promise that all examples are runnable (except for replacing IDs as needed), this is the perfect way to verify since it'll be just like a user copy/pasting the example.
If we can fix these few issues and run each one to ensure it's setup correct, we'll be golden!
| var client = new EasyPost.Client(new EasyPost.ClientConfiguration("EASYPOST_API_KEY")); | ||
| EasyPost.Models.API.Shipment shipment = await client.Shipment.Retrieve("shp_..."); | ||
| shipment = await client.Shipment.Luma( | ||
| shipment.Id, | ||
| rulesetName: "ruleset_name", | ||
| plannedShipDate: "2025-07-03", | ||
| deliverByDate: "2025-07-06" | ||
| ); | ||
| Console.WriteLine(JsonConvert.SerializeObject(shipment, Formatting.Indented)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's split each of these statements by an empty line like other examples are
|
|
||
| func buy() { | ||
| client := easypost.New("EASYPOST_API_KEY") | ||
| shipment, _ := client.CreateShipment( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this is creating a shipment, not buying with luma
|
|
||
| func oneCallBuy() { | ||
| client := easypost.New("EASYPOST_API_KEY") | ||
| shipment, _ := client.CreateShipment( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this is creating a shipment, not buying with luma
|
|
||
| func promise() { | ||
| client := easypost.New("EASYPOST_API_KEY") | ||
| shipment, _ := client.CreateShipment( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this is creating a shipment, not getting the promise with luma
| public static void main(String[] args) throws Exception { | ||
| EasyPostClient client = new EasyPostClient("EASYPOST_API_KEY"); | ||
| Shipment shipment = client.shipment.create( | ||
| Map.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: we don't use Map.of anywhere else in our examples, can we instead set it up like the others? (though, I do like this so maybe holistically we can replace the Java ones to clean them up!)
| EasyPostClient client = new EasyPostClient("EASYPOST_API_KEY"); | ||
| Shipment lumaPromise = client.shipment.lumaPromise( | ||
| Map.of( | ||
| "shipment", Map.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: same as above with Map.of
| const api = new EasyPost('EASYPOST_API_KEY'); | ||
|
|
||
| (async () => { | ||
| const shipment = await api.Shipment.create({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this only creates a shipment, not buys it with luma
| const client = new EasyPostClient('EASYPOST_API_KEY'); | ||
|
|
||
| (async () => { | ||
| const promise = await client.Shipment.lumaPromise({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: lumaPromise is not defined, it's probably client.Luma.promise or similar
Justintime50
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really close! There's a couple of small consistency things, then let's get this merged!
| Weight = 65.9 | ||
| }, | ||
| CarrierAccounts = new List<string> { "ca_..." }, | ||
| PersistLabel = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: with keeping examples as simple as possible, all extra params could(should) be left off - they'll still be defined on the docs but the snippets themselves should contain only what's necessary to get a happy response back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this is why running the example is important 😅
We need carrier accounts for one-call buys, we do not need persist label
| shipment, _ := client.GetShipment("shp_...") | ||
| lumaRequest := &easypost.LumaRequest{ | ||
| Shipment: easypost.Shipment{ID: shipment.ID}, | ||
| RulesetName: "ruleset_...", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: with the consistency, we should have all examples across all libs share the same params - in this case the ruleset name should be defined the same across all
Co-authored-by: Justin Hammond <[email protected]>
Co-authored-by: Justin Hammond <[email protected]>
No description provided.