SwiftLint
SwiftLint copied to clipboard
realm
Breaking change: paths specified on the command line should take precedence over configurations
This PR changes how SwiftLint treats files or directories passed as command line arguments, and resolves #4823
Typically, one would expect that paths explicitly supplied on the command line would take precedence over paths specified in a configuration file.
SwiftLint behaves like this in most cases, but there are some scenarios where the existing behavior is not intuitive or expected.
I'll enumerate some scenarios below (some of which have not changed at all).
-
swiftlint- if there is a configuration file present, this will be attended to. Otherwise, lint all.swiftfiles under the current directory - unchanged -
swiftlint /relative/or/absolute/path/to/some/file- the specified file(s) will be linted - unchanged -
swiftlint /relative/or/absolute/path/to/some/file /relative/or/absolute/path/to/some/file_that_does_not_actually existOld behaviour: SwiftLint would lint the file that exists, and then scan all files and directories specified in the configuration file if one was present. If no configuration file was present, SwiftLint would just lint the existing file.
New behaviour: SwiftLint will ignore the configuration's specified files and directories, and lint any specified files that actually do exist. A warning will be printed for any path supplied on the command line that does not exist:
warning: No file or directory found at '/tmp/Z.swift'. -
swiftlint /relative/or/absolute/path/to/some/directoryOld behaviour - if a configuration file was detected, and specified any directories to scan, the configuration file's specification of which files/directories to scan would be used, and the command line directory (or directories) would be ignored. If no configuration file was present, or if it specified no paths to scan, then the directory specified on the command line would be scanned.
New behaviour - the directory or directories specified on the command line will be scanned. Any directories specified in a configuration file will be ignored. The remainder of the configuration file, if any, for example, exclusions, rule enablement or disablement, will be respected.
Messaging
If there is no configuration file, or no paths are specified in the configuration file, then SwiftLint will print
Linting Swift files in current working directory
as before. However if a configuration file is specified, or the default is present, and any paths are specified in the configuration, then SwiftLint will now print
Linting Swift files specified in the configuration
instead.
| 16 Messages | |
|---|---|
| :book: | Linting Aerial with this PR took 1.19s vs 1.19s on main (0% slower) |
| :book: | Linting Alamofire with this PR took 1.56s vs 1.55s on main (0% slower) |
| :book: | Linting Brave with this PR took 8.95s vs 8.99s on main (0% faster) |
| :book: | Linting DuckDuckGo with this PR took 4.5s vs 4.5s on main (0% slower) |
| :book: | Linting Firefox with this PR took 10.42s vs 10.43s on main (0% faster) |
| :book: | Linting Kickstarter with this PR took 11.03s vs 11.03s on main (0% slower) |
| :book: | Linting Moya with this PR took 0.62s vs 0.62s on main (0% slower) |
| :book: | Linting NetNewsWire with this PR took 3.29s vs 3.27s on main (0% slower) |
| :book: | Linting Nimble with this PR took 0.83s vs 0.81s on main (2% slower) |
| :book: | Linting PocketCasts with this PR took 8.82s vs 8.84s on main (0% faster) |
| :book: | Linting Quick with this PR took 0.4s vs 0.39s on main (2% slower) |
| :book: | Linting Realm with this PR took 11.43s vs 11.45s on main (0% faster) |
| :book: | Linting Sourcery with this PR took 2.84s vs 2.82s on main (0% slower) |
| :book: | Linting VLC with this PR took 1.55s vs 1.56s on main (0% faster) |
| :book: | Linting Wire with this PR took 19.65s vs 19.67s on main (0% faster) |
| :book: | Linting WordPress with this PR took 12.06s vs 13.39s on main (9% faster) |
Generated by :no_entry_sign: Danger
I like the proposed change. However, we have to be very careful with this change. It might break people's workflows.
Absolutely. If someone is inadvertently passing a non-existent file or a directory on the command line in their CI scripts, they would previously get a scan based on their config file settings, whereas now SwiftLint will potentially scan nothing (in the case of a non-existent path), or just the specified directory.
I like the proposed change. However, we have to be very careful with this change. It might break people's workflows.
Absolutely. If someone is inadvertently passing a non-existent file or a directory on the command line in their CI scripts, they would previously get a scan based on their config file settings, whereas now SwiftLint will potentially scan nothing (in the case of a non-existent path), or just the specified directory.
If the passed file doesn't exist, SwiftLint shows a warning now. That's nice. So it's not just silently doing nothing. What would be even better is when SwiftLint was returning a non-zero result code. With that, CI scripts calling SwiftLint would clearly report back if there was nothing to lint. Do you think that's achievable without changing too much?
I like the proposed change. However, we have to be very careful with this change. It might break people's workflows.
Absolutely. If someone is inadvertently passing a non-existent file or a directory on the command line in their CI scripts, they would previously get a scan based on their config file settings, whereas now SwiftLint will potentially scan nothing (in the case of a non-existent path), or just the specified directory.
If the passed file doesn't exist, SwiftLint shows a warning now. That's nice. So it's not just silently doing nothing. What would be even better is when SwiftLint was returning a non-zero result code. With that, CI scripts calling SwiftLint would clearly report back if there was nothing to lint. Do you think that's achievable without changing too much?
I think that should be achievable - I think we will actually return a non zero status right now, if allow zero lintable files is off, purely by chance, but we might want to ignore zero lintable files if paths are supplied and none of them exist/can be read.
I like the proposed change. However, we have to be very careful with this change. It might break people's workflows.
Absolutely. If someone is inadvertently passing a non-existent file or a directory on the command line in their CI scripts, they would previously get a scan based on their config file settings, whereas now SwiftLint will potentially scan nothing (in the case of a non-existent path), or just the specified directory.
If the passed file doesn't exist, SwiftLint shows a warning now. That's nice. So it's not just silently doing nothing. What would be even better is when SwiftLint was returning a non-zero result code. With that, CI scripts calling SwiftLint would clearly report back if there was nothing to lint. Do you think that's achievable without changing too much?
I think that should be achievable - I think we will actually return a non zero status right now, if allow zero lintable files is off, purely by chance, but we might want to ignore zero lintable files if paths are supplied and none of them exist/can be read.
That means the current behavior is already good. Whenever paths specified on the command line or in the configuration result in an empty set of files to lint, SwiftLint fails with a message. Also, your new warning lets users know that the path given doesn't exist.
The allow_zero_lintable_files option can only be specified in a configuration, so it should only apply to sessions initiated by the include/exclude paths defined in the same. The option should not have an influence on anything specified on the command line. This is what you mean, isn't it?
The
allow_zero_lintable_filesoption can only be specified in a configuration, so it should only apply to sessions initiated by the include/exclude paths defined in the same. The option should not have an influence on anything specified on the command line. This is what you mean, isn't it?
Not quite - the way I see it, specifying paths on the command line means that any included paths from the config file, if it's present, should be ignored.
But the other settings in the config file should still be respected (IMO) - e.g. the rules settings, and also, and I think this is right, allow_zero_lintable_files should be respected as well - if I have that setting on in my config, and I specify on the command line directories that exist and are readable, but contain no swift files, I would not expect to see an error - I specified in the config that no swift files was ok.
Similarly, currently, we will respect the excluded settings from the config file.
So even though this "works" in this branch currently, if allow_zero_lintable_files is off, I think we actually want a separate error, which is "paths were supplied on the command line, but none of them were readable (or existed).
I think I have a potential fix for this, which is not too awful - still playing around with it
So this latest commit (https://github.com/realm/SwiftLint/pull/4849/commits/8650b5719c2add62bba8e4f6113190df49d52cfd) will produce a non-zero exit status if none of the supplied paths is exists, or is readable.
(Actually, currently, if a file is not readable, SwiftLint will crash, but there is a fix for that in SourceKitten, that hasn't made it into a release yet).
Not the most graceful - I couldn't see any elegant way to propagate the fact that the path did not exist back up the call tree, so if paths were supplied, but we ended up with no lintable files, we check whether any of the supplied paths exists, or is readable, and if not, throw the error.
Hey Martin, thanks so much for taking this on and for your patience on this.
It feels like ever time I've touched the precedence of included/excluded files in the past, it's angered some user somewhere that was relying on the previous behavior.
One example in which I could foresee this breaking people's SwiftLint workflows is when they have some sort of integration with another tool or are calling SwiftLint from a script. For example, if someone had a shell script that called SwiftLint on all files changed in a pull request, they might still want their configuration to take precedence over file paths that were passed on the CLI.
On the surface, I would tend to agree that a path specified on the command line interface seems like a stronger signal to the tool that the user actually wants to see lint results for that file, regardless of if it's ignored in the configuration, but in practice because of use cases like the one I've mentioned above, this might be counter-intuitive to some folks.
Happy to discuss this further if you like, but for the time being I'm inclined to avoid making this change.
Hey Martin, thanks so much for taking this on and for your patience on this.
It feels like ever time I've touched the precedence of included/excluded files in the past, it's angered some user somewhere that was relying on the previous behavior.
One example in which I could foresee this breaking people's SwiftLint workflows is when they have some sort of integration with another tool or are calling SwiftLint from a script. For example, if someone had a shell script that called SwiftLint on all files changed in a pull request, they might still want their configuration to take precedence over file paths that were passed on the CLI.
On the surface, I would tend to agree that a path specified on the command line interface seems like a stronger signal to the tool that the user actually wants to see lint results for that file, regardless of if it's ignored in the configuration, but in practice because of use cases like the one I've mentioned above, this might be counter-intuitive to some folks.
Happy to discuss this further if you like, but for the time being I'm inclined to avoid making this change.
So I can totally understand the sensitivity around changes like this.
Would you consider a change that fixed case 3. (where a non-existent file path on the command line causes us to fall back to the configuration), but left the directory case (4) alone?