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

Log errors when versions can't be created #809

Merged
merged 3 commits into from May 18, 2016
Merged

Log errors when versions can't be created #809

merged 3 commits into from May 18, 2016

Conversation

md5
Copy link
Contributor

@md5 md5 commented May 18, 2016

As discussed in #807, this PR updates record_update and record_destroy to log a warning listing all validation errors when a Version instance cannot be saved. It does not update record_create since that calls Version.create! instead of Version.create and should raise an error for any validation failures.

Due to Rubocop complaining, I have also slightly refactored has_paper_trail.rb to fix some of the complaints. There is still a complaint about the InstanceMethods module being too many lines, but I didn't see any easy way to reduce the line count of these changes without going too far off topic and into the weeds.

@jaredbeck
Copy link
Member

There is still a complaint about the InstanceMethods module being too many lines ..

Just bump up the number in rubocop_todo.yml .. it's a fight for another day :)

unless log_version_errors(version, :update)
update_transaction_id(version)
save_associations(version)
end
Copy link
Member

Choose a reason for hiding this comment

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

I find this conditional hard to read. I'd prefer:

if version.errors.any?
  log_version_errors(version, :update)
else
  update_transaction_id(version)
  # etc.

If rubocop complains about method complexity, just bump up the numbers; it's a fight for another day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I had originally until RuboCop complained about AbcSize. I've changed it back, though subsequent edits seem to have removed the need to bump AbcSize.

@md5
Copy link
Contributor Author

md5 commented May 18, 2016

I've refactored and bumped the Metrics/ModuleLength limit in .rubycop_todo.yml 👍

@jaredbeck jaredbeck merged commit 7963d63 into paper-trail-gem:master May 18, 2016
@jaredbeck
Copy link
Member

Thanks Mike!

jaredbeck added a commit that referenced this pull request May 18, 2016
@md5 md5 deleted the log-errors branch May 18, 2016 18:55
@md5
Copy link
Contributor Author

md5 commented May 18, 2016

🤘

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

2 participants