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

Fix Style/TrivialAccessors to support AllowPredicates: false #2014

Merged

Conversation

gotrevor
Copy link
Contributor

@gotrevor gotrevor commented Jul 4, 2015

No description provided.

@gotrevor gotrevor force-pushed the TrivalPredicate_AllowPredicates_bugfix branch from 38d7e7f to 0260d38 Compare July 4, 2015 19:23
@gotrevor gotrevor force-pushed the TrivalPredicate_AllowPredicates_bugfix branch from 0260d38 to e771f4b Compare July 4, 2015 19:26
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 4, 2015

Hmm, I'm confused. I didn't even remember we had such option and I have no idea why it's the default. Guess the fact that it didn't work helped for this. I wonder if it's needed at all...

@gotrevor
Copy link
Contributor Author

gotrevor commented Jul 4, 2015

I having the default as false is probably right.
Here's one comment of yours about this:
#326 (comment)

Later in that thread, looks like you merged in the changes in a separate commit.

I don't have a strong opinion on whether this option should stay or go. I just noticed it because, in master, def predicate? is not covered: https://coveralls.io/files/917544955

bbatsov added a commit that referenced this pull request Jul 5, 2015
…s_bugfix

Fix `Style/TrivialAccessors` to support AllowPredicates: false
@bbatsov bbatsov merged commit 2b94201 into rubocop:master Jul 5, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 5, 2015

OK, fair enough. I guess I'll dwell on this more some other time and I should definitely work on the config documentation.

Thanks for spotting and fixing this!

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.

2 participants