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 options to update_attributes method signatures. #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsphm
Copy link
Contributor

@jsphm jsphm commented Jan 29, 2016

my_page.update_attributes!(active: true) throws ArgumentError: wrong number of arguments (2 for 1). This is because slices overrides mongoid's update_attributes method, but does not maintain the same method signature; it expects only one argument, rather than two.

When the slices implementation of update_attributes is called by mongoid's update_attributes!, the wrong number of arguments is passed and an error occurs.

@jsphm jsphm added the WIP label Jan 29, 2016
@jgwhite
Copy link
Contributor

jgwhite commented Jan 29, 2016

So we’re accepting the options arg then ignoring it? (Not saying this is a problem, just checking I’ve understood)

@jsphm
Copy link
Contributor Author

jsphm commented Jan 29, 2016

@jgwhite That's correct. I debated using _ rather than options as the argument name, but decided the latter would save having to check the definition of super to see the purpose of the second parameter.

@bensymonds
Copy link
Contributor

Looks ok to me 👍

@jgwhite
Copy link
Contributor

jgwhite commented Jan 29, 2016

agreed 👍

@bensymonds
Copy link
Contributor

@jsphm is this really still wip? Looks like it's ready to go to me...

@jsphm jsphm removed the WIP label Feb 2, 2016
@jsphm
Copy link
Contributor Author

jsphm commented Feb 2, 2016

Good to go 👍

@calumgunn
Copy link
Contributor

I rebased this, because I'm a good boy.

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.

4 participants