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

Add readonly flag to fetch_reified_items method #57

Conversation

dmrAnderson
Copy link
Contributor

No description provided.

@@ -107,7 +107,23 @@ def test_fetch_reified_items

assert children_hash.is_a?(Hash)

assert children_hash.all?{|k,v| v.all?{|x| x.readonly?} }
assert children_hash.all? { |_k, v| v.all? { |x| x.readonly? } }
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
assert children_hash.all? { |_k, v| v.all? { |x| x.readonly? } }
assert children_hash.values.all?(&:readonly?)


assert children_hash.is_a?(Hash)

assert children_hash.all? { |_k, v| v.all? { |x| x.readonly? } }
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
assert children_hash.all? { |_k, v| v.all? { |x| x.readonly? } }
assert children_hash.values.all?(&:readonly?)

reified_children_hash = {}.with_indifferent_access

reified_parent = nil

snapshot_items.each do |si|
reified_item = si.item_type.constantize.new(si.object)

reified_item.readonly!
reified_item.readonly! if readonly
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use multiline if statement here. Thanks

if readonly
  reified_item.readonly!
end

@westonganger
Copy link
Owner

Looks like a good change. I've suggested a couple of comments

Also can we maybe update the section in the README

As a safety these records have the @readonly = true attribute set on them. If you want to perform any write actions on the returned instances you will have to set @readonly = nil.

reified_parent, reified_children_hash = snapshot.fetch_reified_items

reified_parent.instance_variable_set("@readonly", false)

to:

As a safety these records have the readonly attribute set on them. If you want to perform any write actions on the returned instances you will have to set the readonly attribute to false

reified_parent, reified_children_hash = snapshot.fetch_reified_items(readonly: false)
# OR
reified_parent, reified_children_hash = snapshot.fetch_reified_items
reified_children_hash.first.instance_variable_set("@readonly", false)

README.md Outdated
reified_parent, reified_children_hash = snapshot.fetch_reified_items(readonly: false)
```

# OR
Copy link
Owner

Choose a reason for hiding this comment

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

Please merge the two codeblocks and move this inside the code block

@westonganger westonganger merged commit 6bf1738 into westonganger:master Aug 28, 2024
21 checks passed
@westonganger
Copy link
Owner

Merged thanks for your contribution!

@dmrAnderson dmrAnderson deleted the add-readonly-flag-to-fetch-reified-items branch November 8, 2024 21:14
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.

2 participants