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

GC-1944/Change Lob Address #183

Merged
merged 1 commit into from
Jan 26, 2024
Merged

GC-1944/Change Lob Address #183

merged 1 commit into from
Jan 26, 2024

Conversation

haroutrs
Copy link
Contributor

@haroutrs haroutrs commented Jan 24, 2024

Description

Changing Lob's address within the SDK to the current one: 2261 Market Street STE 5668
San Francisco, CA 94114. Limited changes to address

Story

GC-1944

Related PR's

Generator

Verify

  • Code runs without errors
  • Tests pass with >=85% line coverage

$address_editable->setAddressCity("San Francisco");
$address_editable->setAddressState("CA");
$address_editable->setAddressZip("94114");
$address_editable->setAddressCountry("US");
Copy link
Contributor Author

@haroutrs haroutrs Jan 25, 2024

Choose a reason for hiding this comment

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

Unsure if this is ok, or if I should revert it to the old format and just modify the address. For the rest of these as well.

Copy link

Choose a reason for hiding this comment

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

was there a reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, this was just how it was newly generated

Choose a reason for hiding this comment

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

I think the two are semantically equivalent. The constructor for that model class optionally takes an associative array as a parameter and there are also setters for the fields available. I'm assuming the change were updates to the mustache templates for php since this snippets file was last generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insights Kevin!

2, // limit
includeList, // include
dateCreated, // dateCreated
metadata, // metadata
Copy link
Contributor Author

@haroutrs haroutrs Jan 25, 2024

Choose a reason for hiding this comment

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

New variables added to list in multiple places as well. I can revert this as well to keep the changes limited to address, unsure if this is breaking

Copy link

Choose a reason for hiding this comment

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

Do the tests run in this repo :)? Is there a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could not run the tests at all actually.. there are dependency issues Your lock file does not contain a compatible set of packages. Please run composer update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would take a little bit of work to sort out what exactly is going on and update packages

Choose a reason for hiding this comment

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

Similar thing here ... looks like newer generation templates include examples of passing optional parameters to the list method. These are all just code samples for demonstration purposes of course and they're accurate as far as I can tell.

@haroutrs haroutrs requested a review from bamohan January 25, 2024 17:49
Copy link

@kevinpjones kevinpjones left a comment

Choose a reason for hiding this comment

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

LGTM 👍

$address_editable->setAddressCity("San Francisco");
$address_editable->setAddressState("CA");
$address_editable->setAddressZip("94114");
$address_editable->setAddressCountry("US");

Choose a reason for hiding this comment

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

I think the two are semantically equivalent. The constructor for that model class optionally takes an associative array as a parameter and there are also setters for the fields available. I'm assuming the change were updates to the mustache templates for php since this snippets file was last generated.

2, // limit
includeList, // include
dateCreated, // dateCreated
metadata, // metadata

Choose a reason for hiding this comment

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

Similar thing here ... looks like newer generation templates include examples of passing optional parameters to the list method. These are all just code samples for demonstration purposes of course and they're accurate as far as I can tell.

docs/.DS_Store Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how this got added.. will remove

@haroutrs haroutrs force-pushed the GC-1944/update-lob-address branch 3 times, most recently from 056913d to 9728c0f Compare January 26, 2024 16:56
@haroutrs haroutrs force-pushed the GC-1944/update-lob-address branch from 9728c0f to e43c4c5 Compare January 26, 2024 16:59
@haroutrs haroutrs merged commit 5998d46 into main Jan 26, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants