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

Provide arguments to RecordNotFound #31184

Merged

Conversation

@TheSmartnik
Copy link
Contributor

@TheSmartnik TheSmartnik commented Nov 20, 2017

ActiveRecord::RecordNotFound has an inconsistent API, which led to quite unexpected bug where exception instances in some cases had attributes of primary_key, model name and others don't. This pr solves the issue and adds appropriate arguments to ActiveRecord::RecordNotFound.

@rails-bot
Copy link

@rails-bot rails-bot commented Nov 20, 2017

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@TheSmartnik TheSmartnik force-pushed the TheSmartnik:fix_record_not_found_on_reload branch Nov 20, 2017
@TheSmartnik
Copy link
Contributor Author

@TheSmartnik TheSmartnik commented Nov 24, 2017

@schneems Hi, can you check PR, please?

@@ -88,7 +88,7 @@ def find_by!(arg, *args)
where(arg, *args).take!
rescue ::RangeError
raise RecordNotFound.new("Couldn't find #{@klass.name} with an out of range value",
@klass.name)
@klass.name, @klass.primary_key)

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Nov 24, 2017
Member

Should we pass args as well?

This comment has been minimized.

@TheSmartnik

TheSmartnik Nov 24, 2017
Author Contributor

@prathamesh-sonpatki fourth argument is for ids. Here, however, args can be anything. We can of course try to extract ids from arguments, but i don't think it's worth it tbh.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Nov 25, 2017

A reasoning behind that was understandable, but I believe in this case it would be better to make them required.

Did you see the original PR and linked issue? Making those arguments required means that you can't do this anymore:

expect(MyModel).to receive(:some_method).and_raise(ActiveRecord::RecordNotFound)

I'm trying to understand what this is trying to fix.

@TheSmartnik TheSmartnik force-pushed the TheSmartnik:fix_record_not_found_on_reload branch to ae032ec Nov 25, 2017
@TheSmartnik
Copy link
Contributor Author

@TheSmartnik TheSmartnik commented Nov 25, 2017

@rafaelfranca oh, I didn't look at pr and linked issue, just on a commit message. Thanks.
Made arguments optional again, updated the description.

I'm trying to understand what this is trying to fix.

You mean the whole pr? Or just the act making arguments required?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Nov 25, 2017

The making the arguments required. This PR seems good to me now

@rafaelfranca rafaelfranca merged commit 89a209f into rails:master Nov 25, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate All good!
Details
@TheSmartnik TheSmartnik deleted the TheSmartnik:fix_record_not_found_on_reload branch Dec 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.