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

Implement autocorrection for Rails/ReadWriteAttribute cop. #1539

Merged

Conversation

huerlisi
Copy link
Contributor

This implements autocorrection for the Rails/ReadWriteAttribute cop.

It is my first contribution to rubocop, therefore I'm not sure it fits your coding conventions;-)

@huerlisi
Copy link
Contributor Author

The travis failure seems to be because of the merge of #1538: https://travis-ci.org/bbatsov/rubocop/builds/45374765

end
end

def autocorrect_read(node)
Copy link
Collaborator

Choose a reason for hiding this comment

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

autocorrect_read and autocorrect_write should be private.

@huerlisi huerlisi force-pushed the autocorrect-rails-read-write-attribute branch from 13eab86 to 667c463 Compare January 1, 2015 15:30
@huerlisi
Copy link
Contributor Author

huerlisi commented Jan 1, 2015

@jonas054 thank you for the code review, appreciated!

I've changed the code accordingly and rebased on current master. I've created independent commits for the issues you mentioned. Should I squash them again?

@jonas054
Copy link
Collaborator

jonas054 commented Jan 2, 2015

The code looks good to me now apart from the missing explicit receiver in the specs that I mentioned, so please make that change and squash again. Maybe @bbatsov has some more comments.

@huerlisi huerlisi force-pushed the autocorrect-rails-read-write-attribute branch from 667c463 to 9ef1b40 Compare January 2, 2015 09:23
@huerlisi
Copy link
Contributor Author

huerlisi commented Jan 2, 2015

Squashed the autocorrection commits and added explicit specs.

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 5, 2015

Doesn't look squashed to me.

@huerlisi
Copy link
Contributor Author

huerlisi commented Jan 5, 2015

I've squashed the follow up fixes into the main autocorrection changes. I decidedly let it be 4 commits who each have some other context/target. Didn't want to have additional specs of current behavior and new functionality in the same commit;-)

But I can squash it all together, as you like?

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 6, 2015

@huerlisi The first 3 commits seem like a single task to me - I'd normally have them as a single commit. I'm a believer that related changes should live in a single commit.

@huerlisi huerlisi force-pushed the autocorrect-rails-read-write-attribute branch from 9ef1b40 to 2498982 Compare January 6, 2015 20:53
@huerlisi
Copy link
Contributor Author

huerlisi commented Jan 6, 2015

Sure, makes sense. Squashed as requested.

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 7, 2015

Thanks! 👍

bbatsov added a commit that referenced this pull request Jan 7, 2015
…ttribute

Implement autocorrection for Rails/ReadWriteAttribute cop.
@bbatsov bbatsov merged commit 7650116 into rubocop:master Jan 7, 2015
@huerlisi huerlisi deleted the autocorrect-rails-read-write-attribute branch January 7, 2015 07:10
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.

3 participants