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.

Sign up for GitHub

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

Jump to bottom

Fix Style/Alias offenses #758

Merged
merged 1 commit into from
Apr 8, 2016
Merged

Fix Style/Alias offenses #758

merged 1 commit into from
Apr 8, 2016

Conversation

magni-
Copy link
Contributor

@magni- magni- commented Apr 4, 2016

From #752.

magni-
magni- reviewed Apr 4, 2016
View reviewed changes
lib/paper_trail/has_paper_trail.rb
@@ -252,6 +252,7 @@ def without_versioning(method = nil)

# Utility method for reifying. Anything executed inside the block will
# appear like a new record.
# rubocop: disable Style/Alias
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reposting my original comment here:

But the Alias false positive is an issue with Rubocop not parsing the instance_eval blocks properly. Making Rubocop happy for that case would actually end up breaking things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I submitted a fix to Rubocop here: rubocop/rubocop#3004

@magni-
Copy link
Contributor Author

magni- commented Apr 4, 2016

Not quite sure what is wrong with the build (the only failure is 2.3.0/AR4/Postgres), since everything passes locally for me with that configuration.

@jaredbeck
Copy link
Member

Please rebase.

@jaredbeck
Copy link
Member

Looks good.

All the new aliases are in the same lexical scope as the methods they alias, so if I understand the style guide (https://github.com/bbatsov/ruby-style-guide#alias-method-lexically) we're good there.

Dropping track_associations (without the question mark) could be considered a breaking change, but I doubt anyone's using it and our next release is a major version (5), so I guess it's OK as long as we add a line to the changelog about it.

I don't like inline linter comments, but these are temporary so I can live with it.

Thanks Paul!

@jaredbeck jaredbeck merged commit 9658088 into paper-trail-gem:master Apr 8, 2016
@magni- magni- deleted the rubocop_alias branch April 8, 2016 04:57
magni- added a commit to magni-/paper_trail that referenced this pull request Apr 8, 2016
@magni-
Update changelog for paper-trail-gem#758
b4639c8
jaredbeck added a commit that referenced this pull request Apr 8, 2016
@jaredbeck
Merge pull request #770 from magni-/update_changelog
5dd7448 
Update changelog for #758
@magni- magni- mentioned this pull request May 13, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@magni- @jaredbeck