Skip to content
This repository was archived by the owner on Apr 11, 2024. It is now read-only.

Adds support for read-only attributes to serialize #380

Merged
merged 2 commits into from
May 5, 2022

Conversation

mkevinosullivan
Copy link
Contributor

WHY are these changes introduced?

To align the behaviour of serialize with that of the Ruby to_hash method, as updated by Shopify/shopify-api-ruby#941

WHAT is this pull request doing?

Previously, serialize excluded read-only attributes, since it was assumed it was just being used by save method.

Now, default is that serialize returns all attributes, and an optional (saving=)true parameter will exclude them when used inside save method.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above
  • I have added/updated tests for this change

@mkevinosullivan mkevinosullivan marked this pull request as ready for review May 4, 2022 16:42
@mkevinosullivan mkevinosullivan requested a review from a team as a code owner May 4, 2022 16:42
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 need to take it one step further IMO!

) {
return acc;
}

if (attribute in HAS_MANY && value) {
acc[attribute] = value.reduce(
(attrAcc: Body, entry: Base) =>
attrAcc.concat(entry.serialize ? entry.serialize() : entry),
attrAcc.concat(entry.serialize ? entry.serialize(saving) : entry),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably also do the thing here where if !entry.serialize, we create a new instance so that we can serialize it.

@mkevinosullivan mkevinosullivan force-pushed the kos/add_saving_param_to_serialize branch from 73ff445 to 4512467 Compare May 5, 2022 03:50
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.

🚢

Previously, serialize excluded read-only attributes, since it was
assumed it was just being used by save method.

Now, default is that serialize returns all attributes, and an optional
(saving=)true parameter will exclude them when used inside save method.
@mkevinosullivan mkevinosullivan force-pushed the kos/add_saving_param_to_serialize branch from 082a2c1 to 08815d1 Compare May 5, 2022 13:48
@mkevinosullivan mkevinosullivan merged commit 2c8d5ac into main May 5, 2022
@mkevinosullivan mkevinosullivan deleted the kos/add_saving_param_to_serialize branch May 5, 2022 15:03
@shopify-shipit shopify-shipit bot temporarily deployed to production May 16, 2022 14:32 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to production July 4, 2022 16:01 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants