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

Load unknown REST resource attributes from data #920

Merged
merged 4 commits into from
Apr 8, 2022

Conversation

paulomarg
Copy link
Contributor

@paulomarg paulomarg commented Apr 6, 2022

Description

Sometimes, the API will send back data that's not exactly as documented, such as the optionX fields in products / variants - those are documented as option, but they dynamically accept any number of options.

The current code only accepts fields that are in the "expected" set when loading API data, but in cases like the above, we should just take whatever the API is sending our way.

How has this been tested?

Via a new unit test.

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added a changelog line.

@paulomarg paulomarg marked this pull request as ready for review April 7, 2022 13:56
@paulomarg paulomarg requested a review from a team as a code owner April 7, 2022 13:56
@@ -8,496 +16,499 @@

## Version 9.5

* [#883](https://github.com/Shopify/shopify_api/pull/883) Add support for Ruby 3.0
- [#883](https://github.com/Shopify/shopify_api/pull/883) Add support for Ruby 3.0
Copy link
Contributor Author

@paulomarg paulomarg Apr 7, 2022

Choose a reason for hiding this comment

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

Everything after this line is just linting auto-fixes to the changelog, please ignore!

Comment on lines 336 to 337
clean = var.to_s.gsub(/[\?\s]/, "")
@aliased_properties[var.to_s] = clean if clean != var
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the fields returned by the API have special characters in them - I don't love this solution, but I couldn't figure out a better way of cleaning up the instance attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

i get it, i also dont like it, but i also couldnt figure out another solution :high-five:

Copy link
Contributor

@mllemango mllemango left a comment

Choose a reason for hiding this comment

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

LGTM
I'm having a hard time reading through some of these methods now that I've forgotten what a resource looks like. I wonder if it makes sense to unit test some of these methods as a way of documentation? For example, I had a hard time seeing what create_instance takes in as data or what to expect method_missing does.
(this seems outside of the scope of this PR)

lib/shopify_api/rest/base.rb Show resolved Hide resolved
@paulomarg
Copy link
Contributor Author

I wonder if it makes sense to unit test some of these methods as a way of documentation?

I think it may. Those methods are all indirectly tested by our suite, but if they're hard to grok having a test is probably useful.

@paulomarg paulomarg merged commit bab92a1 into main Apr 8, 2022
@paulomarg paulomarg deleted the load_unknown_attributes branch April 8, 2022 19:56
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems April 11, 2022 17:32 Inactive
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

3 participants