Policy for new enabled-by-default rules

[jshier]

I appreciate the continued development of this tool, as it continues to be better than swift-format. However, the number of default rules added over time have become a major issue. Every release has the potential for dozens to hundreds of changes in a codebase, just by running the formatter. While I recognize this is technically okay by the rules of semver, given the tool is still below 1.0, it's still quite disruptive. Default rules should be reserved for true Swift styling that has near universal adoption already. It should NOT be for highly opinionated rules, like the various wrap* rules that have been added over the last couple of releases, especially when there are no customizations available for those rules.

On the matter of versioning, this tool has been around for quite a while now, so I think it's time to go 1.0 and provide a little bit of stability to the users. As part of that, codifying the criteria for making new default rules would also be a good idea, like SwiftLint.

[jshier]

Additionally, it would be a good idea to improve SwiftFormat's handling of new default rules anyway. I'm not familiar with how other tools do it, but some way of indicating, perhaps on first run, that new rules have been enabled, and allow users to opt out would be very nice.

$ swiftformat .
swiftformat 0.99 enables the following rules by default:
--noNegativeNumbers
--onlyShortStrings
Do you wish to continue? Y/N

Or even just printing the new rule names would be good, as that's often the hardest part of rolling back new default rules.

$ swiftformat .
99/100 files formatted, 0 files skipped.
New default rules applied:
--noNegativeNumbers
--onlyShortStrings

[rgoldberg]

Every violation specifies the rule; you can then disable the rule in your config.

If a rule is so universal it should be enabled by default, then violations alert users to bad practices of which they might be unaware.

I don't see how releases with new, easily disblable rules every 1-3 months is onerous.

[jshier]

I don't use SwiftFormat as a linter, only a formatter, so there's no violation output to print. There's currently no way to find out new default rules when formatting, though I suppose I could revert the formatting (once I extract the changes from my branch), run it in lint mode, and find the rules that way. But formatting tools aren't meant to be disruptive, they're meant to enable productivity by eliminating arguments over formatting. Every time I have to undo the applied format and turn off more rules, something's gone wrong.

Besides, the notion of a "universal" formatting rule is largely subjective, even more so than listing rules (and by and large, SwiftFormat focuses on actual formatting, it doesn't have many rules for "best practices" or dangerous APIs). A part of this process should be codifying rules around when a rule can be made a default, like other tools have.

[rgoldberg]

If you get tons of reformats, it's obvious that a new version of SwiftFormat was released.

Just revert your code in your VCS, lint, and disable rules you don't like (I don't format code that wasn't already locally committed, as it's easy to amend an existing commit).

There were 6 sub-1.0 minor releases in the past year; 5 minutes once every 2 months doesn't seem onerous.

I don't have anything against documented requirements for default rules, default rules leaning away from formatting & more towards functional differences, or a 1.0 version.

[nicklockwood]

@jshier the default rule policy is to aid discoverability. Almost nobody reads the docs for each release to discover which new rules have been added, so most rules would not get used at all if not enabled by default.

This leads to significant problems with beta testing as well. Often when an old rule is enabled by default we get reports of bugs that went undiscovered for years because so few people were using it before.

If you would prefer to opt out of new rules though, you can freeze your .swiftformat config to a specific set of rules by using:

--disable all
--enable rule1
--enable rule2
...etc

[calda]

I agree with Nick's points about discoverability and testing.

We also try to guess whether a new rule will be controversial, in which case we disable it by default. For example, the newer wrapGuardStatementBodies rule was disabled by default because single-line guard statements are a very common pattern. I expect many of the other enabled-by-default rules added this year are welcome additions to most teams (redundantSwiftUIGroup, redundantEmptyView, redundantSwiftTestingSuite, swiftTestingTestCaseNames, etc).

I acknowledge that some of the other new wrapping rules may also be controversial, but I personally stand by them, and there will always be judgement calls here. For example, redundantSelf is still controversial in pockets of the community, but I don't think we would consider not having it enabled by default.

One recent miss was that we were a little over-optimistic with the new rules related to standard library method calls (preferContains, isEmpty, etc), which will break the build if applied to a custom type with only a subset of the standard library methods. We have updated all of these rules to be disabled-by-default: #2596

[calda]

To summarize our general policy:

1. Rules that potentially break the build should usually be disabled-by-default.
2. New rules that seem controversial should be disabled-by-default.
3. Otherwise, new rules will be enabled-by-default. This helps with discoverability and testing.

[LePips]

I don't feel like the issue of disruption was fully addressed.

I'm refacing an issue again I've long had with this project introducing new defaults. I don't disagree with them, but they're defaults. I use GitHub Actions which has lately updated SwiftFormat to 0.63.0 and is currently rolling out the new images. My PRs will be allocated the old or newer image willy nilly so it's annoying to have to just ignore my check, even on my PR performing the updated format from 0.63.0 (because it got a 0.62.1 runner) until there are enough runners with the new image to stabilize.

This is an issue with how constantly updated the default rules are, which is why the original post is asking for some sort of stabilization of rules with major releases that could properly introduce new changes.

[nicklockwood]

This is a fair point. I think we need to offer a simple way for people to opt out of new rules past a certain point in time.

You can specify the set of rules that you want to use, in which case you won't be opted into new ones. But there's currently no way to say "please lock in the current set of rules"

[rgoldberg]

I assume that it would be best to lock default-enabled rules at a specific SwiftFormat version, not based on time. Something like:

swiftformat --baseline 0.60.0

Obviously, --baseline could be renamed. But, keeping it general, instead of something like --default-enabled 0.60.0, allows the baseline to be applied to things beyond just what rules are enabled by default (it could apply to the default values for options, etc., even changed behavior outside of bug fixes could be maintained for backwards compatibility, if you wanted to support the complexity in code; such old code paths could be removed from new versions after a certain period of time has elapsed, like a year, to allow transition time, but to avoid carrying technical debt indefinitely).

[calda]

I use GitHub Actions which has lately updated SwiftFormat to 0.63.0 and is currently rolling out the new images. My PRs will be allocated the old or newer image willy nilly so it's annoying to have to just ignore my check

I think typically this shouldn't add additional disruption. New rules are almost always additive, in which case a codebase formatted with a new SwiftFormat version won't have the changes in those new rules undone by the old version. Once a codebase is formatted with a new version, it should still "pass" the lint check of a previous version. 0.63.0 also didn't include any new enabled-by-default rules, so that is not the issue here.

For this specific case, the problem would be if the two versions apply conflicting formatting to the same code. The only change I see in 0.63.0 that would super obviously cause churn between 0.62.1 is "Updated organizeDeclarations to categorize associatedtype declarations as beforeMarks" (#2668). I could also see there potentially being churn related to the new changes to redundantBackticks and swiftTestingTestCaseNames (#2635), but am not sure without seeing an example.

The organizationDeclarations change fixed a build-breaking edge case in the previous behavior. organizeDeclarations is also not an enabled-by-default rule, so I suspect is not the change you're referring to.

If the churn you're seeing is caused by a different change, could you share an example of what's happening?