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

Set portals when replacing data in Persistable #1

Merged
merged 1 commit into from Mar 10, 2015

Conversation

lannon
Copy link

@lannon lannon commented Mar 10, 2015

I can add specs for the Persistable module to cover this. But, wanted to see if not setting @Portals here was intentional.

@Portals
Copy link

Portals commented Mar 10, 2015

Hello! :P I approve this PR!

@lannon
Copy link
Author

lannon commented Mar 10, 2015

@Portals :)

@mech
Copy link
Owner

mech commented Mar 10, 2015

Nice catch, I have missed hydrating it back. Since the portals have been built, I think it is safe to merge. I don't think a spec is required but if you have one, why not 😄

mech added a commit that referenced this pull request Mar 10, 2015
Set portals when replacing data in Persistable
@mech mech merged commit ad5c165 into mech:master Mar 10, 2015
@lannon
Copy link
Author

lannon commented Mar 10, 2015

thanks @mech. i haven't actually written any specs. was just puzzling over this late last night and figured it was just an oversight.

@lannon
Copy link
Author

lannon commented Mar 10, 2015

i was thinking that https://github.com/mech/filemaker-ruby/blob/master/lib/filemaker/model/builder.rb could be refactored such that it could be used to handle https://github.com/mech/filemaker-ruby/blob/master/lib/filemaker/model/persistable.rb#L94-L108. may have a PR for that in a little while.

@mech
Copy link
Owner

mech commented Mar 10, 2015

That's wonderful @lannon, 👍 for refactoring them to be together so they are easy to reason about.

@lannon lannon deleted the set_portals_in_persistence branch March 10, 2015 20:35
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