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

Remove SafeMode from Count and Detect #72

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

rrosenblum
Copy link
Contributor

@rrosenblum rrosenblum commented Jul 18, 2019

This is in response to #69. As noted in the issue, SafeMode has outlived its usefulness. Making use of being disabled by default or setting this as unsafe for auto-correction make more sense.

I have left in the functionality that automatically disables these cops when the Rails cops are enabled. I'm not sure if that is functionality that we would like to maintain or not. These are the only 2 cops that function this way. If we remove the functionality, we can remove the SafeMode module from the core project.

@koic
Copy link
Member

koic commented Jul 26, 2019

I have left in the functionality that automatically disables these cops when the Rails cops are enabled. I'm not sure if that is functionality that we would like to maintain or not.

Yeah, I'd like to investigate whether rails? method can be removed. Anyway, I agree with SafeMode should be replaced by SafeAutoCorrect.

CHANGELOG.md Outdated Show resolved Hide resolved
@rrosenblum
Copy link
Contributor Author

Yeah, I'd like to investigate whether rails? method can be removed.

I think there is an argument for removing it based on having SafeAutoCorrect set to false and having the reasons why it is considered unsafe being well documented.

@rrosenblum
Copy link
Contributor Author

Should I update the VersionChanged on these cops?

@bbatsov
Copy link
Contributor

bbatsov commented Jul 26, 2019

Yes.

@rrosenblum
Copy link
Contributor Author

@bbatsov what are your thoughts on removing the rails? method in favor of SafeAutoCorrect: false and documentation on why it is unsafe?

@bbatsov
Copy link
Contributor

bbatsov commented Jul 29, 2019

That sounds like a good approach to me. Let's make it happen!

@rrosenblum
Copy link
Contributor Author

Cool, I'll update this code to remove the rails? check. I'll then create a PR that removes the SafeMode module from RuboCop core.

@rrosenblum
Copy link
Contributor Author

@koic this is ready for review again.

CHANGELOG.md Outdated Show resolved Hide resolved
@koic koic merged commit 0e168d2 into rubocop:master Aug 19, 2019
@rrosenblum rrosenblum deleted the remove_safe_mode branch August 19, 2019 14:00
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