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

v1 default: Include Safe: false by default #8156

Closed
marcandre opened this issue Jun 15, 2020 · 7 comments
Closed

v1 default: Include Safe: false by default #8156

marcandre opened this issue Jun 15, 2020 · 7 comments
Milestone

Comments

@marcandre
Copy link
Contributor

@bbatsov wrote in #5980 about --safe-auto-correct

Let's implement the metadata and the options firsts, changing the defaults is easy afterwards. 😄

Now would be a good time to ask ourselves if the current default is the right one. safe or unsafe by default?

rubocop -a does all autocorrections, including unsafe ones. One has to add --safe-auto-correct to exclude unsafe ones.

Should this be the default, or should it be the reverse?

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 15, 2020

That's a fair question. I guess by now most unsafe cops (at least those that are really problematic) have been marked as unsafe. I'm afraid that if we make unsafe the default, we'll have to make every existing cop without a default safe anyways, otherwise this would be a huge changes of the current behaviour (unless someone wants to carefully audit each cop, that is).

@marcandre
Copy link
Contributor Author

I'm afraid that if we make unsafe the default

Are you talking about the default for Cops? Or for the CLI? The Cops are Safe by default, but the CLI is currently unsafe by default. I assumed that assuming Cops to be safe by default was ok, my question is about if the CLI rubocop -a should include or exclude unsafe cops by default...

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 16, 2020

Ops, I got confused then. I thought we were discussing the cops in general. For the CLI probably safe auto-correct is a better default, although I'm wary of adding something named unsafe-auto-correct as it would probably scare people too much. :-)

@marcandre
Copy link
Contributor Author

marcandre commented Jun 16, 2020

How about --exclude-unsafe and --include-unsafe, and -a is --exclude-unsafe by default and -A is --include-unsafe by default?
A bit like git branch -d vs git branch -D

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 22, 2020

Sounds reasonable to me.

marcandre added a commit to marcandre/rubocop that referenced this issue Jun 23, 2020
…ault.

`-a` / `--auto-correct` are now safe only, replacing `--safe-auto-correct`
`-A` / `--auto-correct-all` apply safe and unsafe corrections
@paul
Copy link

paul commented Aug 7, 2020

This broke the default autofix for Style/FrozenStringLiteralComment. I've tried hunting around, and I haven't been able to figure out why this one is "unsafe".

Also, this change broke a few other tools like Ale dense-analysis/ale#3237, I'm unable to make this work until the fix their default. Is there something I can do in the meantime in the .rubocop.yml to either allow it to run/autofix "unsafe" cops, or mark this particular cop as "safe" so it can be run?

@marcandre
Copy link
Contributor Author

Sorry for the troubles you are having @paul. Thanks for reaching out.

You can override the settings per cop with Safe: true and SafeAutoCorrect: true, in your .rubocop.yml:

Style/FrozenStringLiteralComment:
  Safe: true
  SafeAutoCorrect: true

There are a few open issues that I hope will make it more obvious what is going on. We are planning on documenting why a cop isn't safe (e.g. Style/FrozenStringLiteralComment is that str = 'silly'; str << ' example' would fail...). We also want to show how many "unsafe" auto-corrections could be additionally be applied.

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

No branches or pull requests

3 participants