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

#311 Don't insert invalid ActiveRecord objects #312

Merged
merged 2 commits into from
Jun 5, 2018

Conversation

seanabrahams
Copy link
Contributor

Initial fix for #311

@brendon
Copy link
Owner

brendon commented May 25, 2018

Hi Sean, do you think insert_at should maybe raise an error rather than return false if used on a new object? I suppose the way you've done it means that it behaves similarly to setting the position property and then saving.

@seanabrahams
Copy link
Contributor Author

Good question. I think we can add an insert_at! method that can raise an error. This way we follow Rails convention. What do you think?

@brendon
Copy link
Owner

brendon commented May 27, 2018

Yes good call on that :) Then that gives users the choice. Once that's done I'll merge this. :)

@brendon
Copy link
Owner

brendon commented May 29, 2018

Thanks @seanabrahams, that looks ok to me. We will have to drop the keyword arguments though since the test suite still supports Ruby 1.9. There also seem to be some other intermittent failures in there.

@swanandp, what's your view on Ruby 1.9 support now? I guess it depends on if we want to support Rails 3.2 anymore.

@seanabrahams
Copy link
Contributor Author

No worries if it needs to be refactored to not use keyword arguments. Passing a method argument down the call chain isn't the most elegant implementation but it looked like significant re-engineering would be required otherwise. Happy to discuss alternatives.

@brendon
Copy link
Owner

brendon commented May 30, 2018

Thanks @seanabrahams, I think yes remove the keyword arguments for now. That's something to look at in a more major refactor. Then lets see how the test suite goes. :)

@brendon
Copy link
Owner

brendon commented Jun 5, 2018

Sorry @seanabrahams, I didn't see your update. I think you have to comment for me to be notified again :) This looks good and it looks like the test suite is fixed too. I'll merge this now.

@brendon brendon merged commit 4773a14 into brendon:master Jun 5, 2018
@brendon
Copy link
Owner

brendon commented Jun 5, 2018

Closes #311

@brendon
Copy link
Owner

brendon commented Jun 5, 2018

Released as 0.9.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