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

Adds --working-directory command line option #5560

Merged
merged 8 commits into from
Jun 29, 2024

Conversation

mildm8nnered
Copy link
Collaborator

@mildm8nnered mildm8nnered commented May 4, 2024

Adds a --working-directory argument to lint and analyze, for users who cannot easily control the current directory when executing SwiftLint (e.g. in some plugin and CI scenarios).

Addresses #5424

This is required because .swiftlint.yml's in the current directory are treated slightly differently to .swiftlint.yml's found deeper in the tree.

For example:

$ pwd
/Users/some.user/Documents/Source/SwiftLint
$
$ swiftlint --quiet --reporter summary
Linting Swift files in current working directory
+-----------------+--------+-------------+--------+----------+--------+------------------+-----------------+
| rule identifier | opt-in | correctable | custom | warnings | errors | total violations | number of files |
+-----------------+--------+-------------+--------+----------+--------+------------------+-----------------+
| Total           |        |             |        |        0 |      0 |                0 |               0 |
+-----------------+--------+-------------+--------+----------+--------+------------------+-----------------+
$
$
$ cd ../..
$ pwd
/Users/some.user/Documents
$
$
$ swiftlint  --quiet --reporter summary Source/SwiftLint
Linting Swift files at paths Source/SwiftLint
+---------------------+--------+-------------+--------+----------+--------+------------------+-----------------+
| rule identifier     | opt-in | correctable | custom | warnings | errors | total violations | number of files |
+---------------------+--------+-------------+--------+----------+--------+------------------+-----------------+
| file_name           | yes    | no          | no     |       18 |      0 |               18 |              18 |
| type_name           | no     | no          | no     |        8 |      0 |                8 |               8 |
| file_header         | yes    | no          | no     |        5 |      0 |                5 |               5 |
| trailing_comma      | no     | yes         | no     |        4 |      0 |                4 |               1 |
| empty_xctest_method | yes    | no          | no     |        1 |      0 |                1 |               1 |
| legacy_objc_type    | yes    | no          | no     |        1 |      0 |                1 |               1 |
| let_var_whitespace  | yes    | no          | no     |        1 |      0 |                1 |               1 |
+---------------------+--------+-------------+--------+----------+--------+------------------+-----------------+
| Total               |        |             |        |       38 |      0 |               38 |              21 |
+---------------------+--------+-------------+--------+----------+--------+------------------+-----------------+
Done linting! Found 38 violations, 0 serious in 646 files.
$
$
$ swiftlint --quiet --reporter summary --working-directory Source/SwiftLint 
Linting Swift files in current working directory
+-----------------+--------+-------------+--------+----------+--------+------------------+-----------------+
| rule identifier | opt-in | correctable | custom | warnings | errors | total violations | number of files |
+-----------------+--------+-------------+--------+----------+--------+------------------+-----------------+
| Total           |        |             |        |        0 |      0 |                0 |               0 |
+-----------------+--------+-------------+--------+----------+--------+------------------+-----------------+

We could assume that the first .swiftlint.yml we encountered was the "main" one, but this would not be true in the unusual, but valid case, where users intentionally did not have a "main" .swiftlint.yml. Additionally, some SwiftLint code, specifically some reporters, assume that the current directory is also the top-level directory of the project.

All relative paths on the command line and in configuration files will be treated as relative to the specified working directory.

@SwiftLintBot
Copy link

SwiftLintBot commented May 4, 2024

17 Messages
📖 Linting Aerial with this PR took 1.02s vs 0.98s on main (4% slower)
📖 Linting Alamofire with this PR took 1.37s vs 1.36s on main (0% slower)
📖 Linting Brave with this PR took 8.04s vs 8.03s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 4.39s vs 4.42s on main (0% faster)
📖 Linting Firefox with this PR took 11.35s vs 11.34s on main (0% slower)
📖 Linting Kickstarter with this PR took 10.07s vs 10.06s on main (0% slower)
📖 Linting Moya with this PR took 0.56s vs 0.56s on main (0% slower)
📖 Linting NetNewsWire with this PR took 2.77s vs 2.75s on main (0% slower)
📖 Linting Nimble with this PR took 0.82s vs 0.81s on main (1% slower)
📖 Linting PocketCasts with this PR took 8.68s vs 8.68s on main (0% slower)
📖 Linting Quick with this PR took 0.45s vs 0.46s on main (2% faster)
📖 Linting Realm with this PR took 5.02s vs 5.05s on main (0% faster)
📖 Linting Sourcery with this PR took 2.53s vs 2.51s on main (0% slower)
📖 Linting Swift with this PR took 4.93s vs 4.92s on main (0% slower)
📖 Linting VLC with this PR took 1.33s vs 1.34s on main (0% faster)
📖 Linting Wire with this PR took 19.22s vs 19.25s on main (0% faster)
📖 Linting WordPress with this PR took 14.21s vs 14.22s on main (0% faster)

Generated by 🚫 Danger

@@ -26,6 +26,11 @@ enum LintOrAnalyzeMode {

struct LintOrAnalyzeCommand {
static func run(_ options: LintOrAnalyzeOptions) async throws {
if let workingDirectory = options.workingDirectory {
if !FileManager.default.changeCurrentDirectoryPath(workingDirectory) {
throw Issue.couldNotChangeToWorkingDirectory(path: workingDirectory)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So originally this throw produced the following eventual output:

Error: couldNotChangeToWorkingDirectory(path: "Source/SwiftLintt")

because the thrown error eventually gets passed to MessageInfo(error: Error, type: ParsableArguments.Type) from the swift-argument-parser. Ideally it would get caught there by

      case let error as LocalizedError where error.errorDescription != nil:
        self = .other(message: error.errorDescription!, exitCode: .failure)

but it was not, because Issue's implementation of errorDescription (var errorDescription: String) does not match the LocalizedError protocol requirement (var errorDescription: String?).

I've fixed that, but because of the way the thrown error is now printed, we get

Error: warning: Could not change working directory to 'Source/SwiftLintt'.

One way around that would just be to brute-force:

if !FileManager.default.changeCurrentDirectoryPath(workingDirectory) {
    Issue.couldNotChangeToWorkingDirectory(path: workingDirectory).print()
    exit(2)
}

Another possibility would be to remove our warning: and error: prefixes from errorDescription, but to add them back in Issue.print() - most usages look like that would be ok, but I'm not sure how confident I am that all of them would be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to SwiftLintError.usageError, which produces: `Error: Could not change working directory to 'Source/SwiftLintt'

I would kind of expected. throwing an Issue to work though.

@mildm8nnered mildm8nnered changed the title Mildm8nnered working directory Adds --working-directory command line option May 4, 2024
@@ -235,7 +235,7 @@ public struct Configuration {
if useDefaultConfigOnFailure ?? !hasCustomConfigurationFiles {
// No files were explicitly specified, so maybe the user doesn't want a config at all -> warn
queuedPrintError(
"\(Issue.wrap(error: error).errorDescription) – Falling back to default configuration"
"\(Issue.wrap(error: error).errorDescription ?? "") – Falling back to default configuration"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what a good default is here

@mildm8nnered mildm8nnered marked this pull request as ready for review May 4, 2024 18:27
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-working-directory branch from 140674c to 5d44cb4 Compare May 11, 2024 13:09
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-working-directory branch 4 times, most recently from 508ac7f to 4eb238d Compare June 28, 2024 00:55
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-working-directory branch from 6d9a9ad to 99fa889 Compare June 28, 2024 18:11
@mildm8nnered mildm8nnered merged commit 593bb62 into realm:main Jun 29, 2024
12 checks passed
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.

None yet

3 participants