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

Fixes to_hash to include readonly attributes #941

Merged

Conversation

mkevinosullivan
Copy link
Contributor

Description

Fixes issue #930

Adds parameter to to_hash so that default behaviour includes readonly attributes; setting the parameter to true indicates that to_hash is being used to serialize the object for saving (and thus excludes readonly attributes).

How has this been tested?

Added tests to verify that readonly attributes are included when using the default to_hash and excluded when using to_hash(true) (i.e, saving = true).

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.

@mkevinosullivan mkevinosullivan requested a review from a team as a code owner April 22, 2022 20:52
@mkevinosullivan mkevinosullivan force-pushed the kos/fix930_variant_tohash_include_inventory_quantity branch from a49146f to 143c24d Compare April 22, 2022 21:32
Copy link
Contributor

@eliasrumley eliasrumley left a comment

Choose a reason for hiding this comment

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

Code looks good, but a thought on the default value

lib/shopify_api/rest/base.rb Show resolved Hide resolved
@mkevinosullivan mkevinosullivan force-pushed the kos/fix930_variant_tohash_include_inventory_quantity branch from 143c24d to 3cb802b Compare April 25, 2022 18:41
Adds parameter to `to_hash` so that default behaviour includes readonly
attributes; setting the parameter to `true` indicates that `to_hash` is
being used to serialize the object for saving (and thus excludes
readonly attributes).

Fixes issue #930
@mkevinosullivan mkevinosullivan force-pushed the kos/fix930_variant_tohash_include_inventory_quantity branch from 3cb802b to 8b3b27a Compare April 26, 2022 15:53
Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

LGTM, just had nits.

lib/shopify_api/rest/base.rb Outdated Show resolved Hide resolved
lib/shopify_api/rest/base.rb Outdated Show resolved Hide resolved
@mkevinosullivan mkevinosullivan force-pushed the kos/fix930_variant_tohash_include_inventory_quantity branch from 9e8dc97 to 31dea0a Compare April 26, 2022 19:03
@mkevinosullivan mkevinosullivan force-pushed the kos/fix930_variant_tohash_include_inventory_quantity branch from 31dea0a to 3634cf7 Compare April 26, 2022 19:28
Copy link
Contributor

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

Great refactorings!

@mkevinosullivan mkevinosullivan merged commit 4da50e4 into main Apr 27, 2022
@mkevinosullivan mkevinosullivan deleted the kos/fix930_variant_tohash_include_inventory_quantity branch April 27, 2022 20:07
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems May 18, 2022 16:29 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems July 4, 2022 16:27 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.

3 participants