Suggestion: rename --ci option to a context-neutral name

[fvsch]

The --ci option is ambiguous:

• Continuous integration covers a wide range of practices.
• It suggests that this command should only be ran on CI servers, but it might be useful to run it on developer workstations as well.
• It doesn't actually describe what the option does.

If the functionality is "check code against the lock file and error out if there is any new linting output", maybe a name like --check might be a better fit?

Edit: other brainstormed names: --test, --strict.

[fvsch]

Another option would be a design where the following behaviors are separated:

1. erroring out on new linting output
2. how to modify the lock file: no modification, bookkeeping and removing issues only, or adding new issues

I think that erroring out if there are new issues could be the default, so that the --ci or --strict option isn't even needed. There could be options for managing the lock file update mode.

Something like:

# Default: errors out on new issues, updates lock file
# with positive changes if there are no new issues
rubocop-gradual

# For CI, might be useful to disable updating so that
# it doesn't modify files
rubocop-gradual --no-update

# Forcing updates, should never error out
# (except for errors in rubocop-gradual, IO errors, etc.)
rubocop-gradual --force-update

(This also suggests renaming --update to --force-update because the default behavior updates the lock file already. Maybe that "update" word is ambiguous as well. ^^)

In that model, I’m a bit unclear if rubocop-gradual should only update the lock file with neutral (bookkeeping) and positive (issue removal) changes if there were also no new issues (meaning that the lock file stays untouched if there is any new issue), or if it should still update the lock file with neutral/positive changes and error out. I think Betterer might take the second option?

[skryukov]

Hey, @fvsch thanks for the issue!

Overall, I agree to your suggestions. Love the --force-update, but it seems to me unclear that rubocop-gradual --no-update returns error even when positive changes occurs. I assume this option would mean --dry-run 🤔 Maybe we can refer what we are trying to do with --ci option: --assert-lock-file or something like this?

# Default: errors out on new issues, updates lock file
# with positive changes if there are no new issues
rubocop-gradual

# Disable updating so that it doesn't modify files,
# but still returns the same error code
rubocop-gradual --dry-run

# For CI, checks if lock file is updated
rubocop-gradual --assert-lock-file

# Forcing updates, should never error out
# (except for errors in rubocop-gradual, IO errors, etc.)
rubocop-gradual --force-update

In that model, I’m a bit unclear if rubocop-gradual should only update the lock file with neutral (bookkeeping) and positive (issue removal) changes if there were also no new issues (meaning that the lock file stays untouched if there is any new issue), or if it should still update the lock file with neutral/positive changes and error out. I think Betterer might take the second option?

I think it should update only when there are no errors because partial update might lead to overuse of force updating:

• user moves a code with offense to another file
• runs rubocop-gradual, which removes that offense from the lock file, but errors out for a "new" offense
• at this point, user can't undo that action without force updating lock file

[fvsch]

Still brainstorming a bit.

(Sorry for blowing up the scope of this issue, but since it's early days for this tool it can be fun to geek out about the API design ^^)

Proposed design

# Default: runs rubocop, compares its output to the rubocop-gradual lock file,
# and shows a report with improvements (fixed issues) and regressions (new issues).
# Does NOT modify the lock file.
rubocop-gradual

# Checks that linting output strictly matches the rubocop-gradual lock file.
# Exits with an error code for any change that is unaccounted for:
# regressions *and* improvements.
# We recommend using this option in CI.
rubocop-gradual --check

# Update the lock file to account for improvements (if any).
# If there is a regression (new issue): exits with an error code,
# and does not touch the lock file.
rubocop-gradual --update

# Update the lock file to match the current linting output, including regressions.
# Only use exceptionally, and review changes to the lock file to make sure you are
# not adding unwanted regressions.
rubocop-gradual --force-update

Some highlights:

• The default behavior is a kind of --dry-run. Personally I dislike how Betterer updates its lock file silently, and I’d rather explicitly ask the tool to write changes, hence the --update option.
• Provides a couple options for updating: --update for the normal case, and --force-update if you need to add regressions to the lock file.
• Provides a single option for CI (or local testing): --check (possible alternative name: --test; I like "check" a little bit more, but both are fine).

Exit codes

I think exit codes would work like this:

command	on regressions	on improvements
rubocop-gradual	1	0
rubocop-gradual --check	1	1
rubocop-gradual --update	1	0
rubocop-gradual --force-update	0	0

(Question: should rubocop-gradual error out on regressions, if it's meant only for showing a report?)

With those exit codes:

1. The --check option is a good fit for CI, and optionally for git pre-push hooks (you can cancel a push if it fails, if you want to be strict or get ahead of CI failures).
2. The --update option can be used in git pre-push or even pre-commit hooks, to transparently update the lock file with improvements, and cancel the commit or push if there is a regression.

Question: loose checking in CI?

My only concern with this design is whether users would like to do more permissive checking in CI.

We use Betterer at StackBlitz and it's sometimes frustrating in our development workflow because of its strict checking. Scenario:

1. As a developer, you make changes to a few source files. Your IDE shows you linting errors in the code you modify or just around it, and you fix a few of those errors without introducing new ones.
2. You forget to update the Betterer lock file.
3. You submit a PR.
4. CI checks the code in your PR against the Betterer lock file, and issues in the lock file that do not exist in the code anymore (aka improvements), and returns an error code: your check fails.

If we used more permissive checking, where only new issues (and probably existing issues that got moved around) would fail checks in CI, the results would be:

• Same efficiency for not introducing new issues.
• Less frustrating experience, short term.
• Medium term, maybe few PRs would actually update the lock file, and it could grow stale, generating false reports when run in development ("Congrats, you removed 25 issues!" when you did no such thing…).

I’m not sure what's the best trade-off here. Should there be no loose checking option because it leads to bad results over time? Or should there be an additional option that is explicitly for loose checking, say --loose-check or --check-regressions?

(Note that in this proposal, both rubocop-gradual and rubocop-gradual --update can be used for loose checking, though they are not intended for that. Maybe that's enough support for loose checking: you can do it if you really want to, but it's not recommended.)

[fvsch]

After a bit more discussion, the idea is to keep rubocop-gradual without options as a command that is feature-full, can error out and have side-effects, because that's a common pattern in the Ruby world (opinionated defaults).

So, keeping rubocop-gradual as a command with side-effects, intended for the main development workflow:

1. The main change would be to rename --ci to something like --check or --test.
2. It might be useful to rename --update to --force-update, and to remove the short -u version, because:
   • if the default command already updates the lock file, --update is ambiguous
   • it might be a useful signal to use that "force" keyword, since it signals that it should be a somewhat exceptional or rare occurrence?
   • and if it should be exceptional or rare, maybe having a short -u version is a footgun?
3. I think a --dry-run (or --no-update) option is not needed (again, given ecosystem conventions), at least initially.
   • If you need to check the lock file against the current codebase, there's the --check option for that already, and we don't want to incentivise doing some form of loose checking.
   • If there's a strong need, there will be feature requests later on.

So I would consider this issue solved if the --ci option gets renamed to something more neutral (and explicit about what it does).

I’ll open another issue about the --update option.