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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

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 0.81s vs 0.8s on main (1% slower)
📖 Linting Alamofire with this PR took 1.14s vs 1.11s on main (2% slower)
📖 Linting Brave with this PR took 6.66s vs 6.62s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 3.47s vs 3.46s on main (0% slower)
📖 Linting Firefox with this PR took 9.44s vs 9.63s on main (1% faster)
📖 Linting Kickstarter with this PR took 8.27s vs 8.36s on main (1% faster)
📖 Linting Moya with this PR took 0.47s vs 0.48s on main (2% faster)
📖 Linting NetNewsWire with this PR took 2.41s vs 2.34s on main (2% slower)
📖 Linting Nimble with this PR took 0.67s vs 0.67s on main (0% slower)
📖 Linting PocketCasts with this PR took 6.81s vs 6.99s on main (2% faster)
📖 Linting Quick with this PR took 0.33s vs 0.32s on main (3% slower)
📖 Linting Realm with this PR took 4.27s vs 4.2s on main (1% slower)
📖 Linting Sourcery with this PR took 2.1s vs 2.09s on main (0% slower)
📖 Linting Swift with this PR took 3.91s vs 3.87s on main (1% slower)
📖 Linting VLC with this PR took 1.13s vs 1.1s on main (2% slower)
📖 Linting Wire with this PR took 14.57s vs 14.27s on main (2% slower)
📖 Linting WordPress with this PR took 9.6s vs 9.83s on main (2% 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
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

2 participants