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

Using --strict --fix hides errors and succeeds #5387

Open
2 tasks done
ladislas opened this issue Dec 11, 2023 · 8 comments · Fixed by #5493
Open
2 tasks done

Using --strict --fix hides errors and succeeds #5387

ladislas opened this issue Dec 11, 2023 · 8 comments · Fixed by #5493
Labels
enhancement Ideas for improvements of existing features and rules. help Questions or user problems that require more explanation rather than code changes.

Comments

@ladislas
Copy link

New Issue Checklist

Describe the bug

We are using swiftlint with pre-commit with the command provided in the documentation

  - repo: https://github.com/realm/SwiftLint
    rev: 0.54.0
    hooks:
      - id: swiftlint
        entry: swiftlint --fix --strict

It does fix the issues but it does not fail.

changing to just swiftlint fails as expected but doesn't fix the issues

Complete output when running SwiftLint, including the stack trace and command used

with swiftlint --fix --strict

$ gc -a

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
check json...........................................(no files to check)Skipped
check for added large files..............................................Passed
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
detect private key.......................................................Passed
forbid submodules....................................(no files to check)Skipped
mixed line ending........................................................Passed
SwiftLint................................................................Passed
SwiftFormat..............................................................Passed

just with swiftlint

$ git commit -a
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
check json...........................................(no files to check)Skipped
check for added large files..............................................Passed
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
detect private key.......................................................Passed
forbid submodules....................................(no files to check)Skipped
mixed line ending........................................................Passed
SwiftLint................................................................Failed
- hook id: swiftlint
- exit code: 2

Linting Swift files at paths Modules/DesignKit/Sources/Colors.swift
Linting 'Colors.swift' (1/1)
/Users/ladislas/dev/leka/ios-monorepo/Modules/DesignKit/Sources/Colors.swift
⛔️ Line 10: Variable name 'arr' should be between 4 and 40 characters long (identifier_name)
⛔️ Line 8: Type name 'leka' should start with an uppercase character (type_name)
Done linting! Found 2 violations, 2 serious in 1 file.

SwiftFormat..............................................................Passed

Environment

  • SwiftLint version (run swiftlint version to be sure)?
swiftlint version
0.53.0
  • Installation method used (Homebrew, CocoaPods, building from source, etc)?

Homebrew

  • Paste your configuration file:
# Leka - iOS Monorepo
# Copyright 2023 APF France handicap
# SPDX-License-Identifier: Apache-2.0

opt_in_rules: []

trailing_comma:
  mandatory_comma: true

type_name:
  min_length: 4 # only warning
  max_length: # warning and error
    warning: 40
    error: 50
  allowed_symbols: ["_"] # these are allowed in type names
  excluded:
    - i18n
    - l10n

identifier_name:
  min_length: # only min_length
    error: 4 # only error
  excluded:
    - a
    - app
    - args
    - b
    - ble
    - BLE
    - cmd
    - g
    - i
    - i18n
    - id
    - img
    - key
    - l10n
    - lhs
    - log
    - new
    - r
    - red
    - rgb
    - rhs
    - rx
    - tx
    - URL

line_length:
  warning: 150
  ignores_urls: true
  ignores_function_declarations: true
  ignores_interpolated_strings: true

disabled_rules:
  - opening_brace
  - switch_case_alignment
  - closure_parameter_position

excluded:
  - Tuist/Dependencies
  - ./*/Derived

reporter: "emoji"

no

  • Which Xcode version are you using (check xcodebuild -version)?
xcodebuild -version
Xcode 15.0.1
Build version 15A507
  • Do you have a sample that shows the issue? Run echo "[string here]" | swiftlint lint --no-cache --use-stdin --enable-all-rules
    to quickly test if your example is really demonstrating the issue. If your example is more
    complex, you can use swiftlint lint --path [file here] --no-cache --enable-all-rules.

It involves using pre-commit, so no

@ladislas
Copy link
Author

funny thing is that using swiftlint --strict, doesn't change all the warnings to errors, I only see the two errors I already have with swiftlint alone

@SimplyDanny
Copy link
Collaborator

Linting and fixing are two different modes. With --fix, SwiftLint only reports corrected violations. They don't fail. That said, --strict doesn't have any effect when auto-correcting code.

For which rules specifically does no promotion happen when running swiftlint --strict?

@SimplyDanny SimplyDanny added the help Questions or user problems that require more explanation rather than code changes. label Dec 15, 2023
@JaviSoto
Copy link
Contributor

JaviSoto commented Mar 11, 2024

Example:

Modify 2 files:

  • A has a violation of rule #1, which is correctable.
  • B has a violation of rule #2, which isn't correctable.

Run:

swiftlint lint --fix --strict

Behavior:

  • A violation is fixed
  • B violation isn't reported
  • swiftlint exists with non-0 code.

If you run the same command again, however, B isn't reported, and swiftlint exists with a 0 code, despite the fact that there's a linting issue.

I believe this may be what @ladislas was pointing out.

@SimplyDanny
Copy link
Collaborator

--fix fixes what's fixable. Nothing else is reported. Thus, --strict doesn't have an effect.

A check could be added that warns about --strict not having any effect if it comes piggyback with --fix.

swiftlint exists with non-0 code.

I cannot see that. swiftlint --fix [--strict] always finishes with 0.

@SimplyDanny SimplyDanny added the enhancement Ideas for improvements of existing features and rules. label Mar 11, 2024
@SimplyDanny SimplyDanny reopened this Mar 11, 2024
@SimplyDanny
Copy link
Collaborator

I'll keep this open awaiting your feedback.

@ladislas
Copy link
Author

I believe this may be what @ladislas was pointing out.

@JaviSoto yes exactly! thanks for clarifying :)

--fix fixes what's fixable. Nothing else is reported. Thus, --strict doesn't have an effect.

@SimplyDanny that makes it clear!

any reason why they don't work together?

@JaviSoto
Copy link
Contributor

A check could be added that warns about --strict not having any effect if it comes piggyback with --fix.

This is nice so at least the user knows this is going on, but it's still undesireable behavior: if you're using --strict it's because you want it to fail if it has any issues. Using --fix should mean "fix anything you can", but given that a lot of the rules can't be fixed, it shouldn't just swallow errors. Because of this behavior we're having to run Swiftlint twice, once with --fix and one without, which is very slow in our codebase. We'd like to be able to run it just once and have it fail if there are any outstanding issues

@SimplyDanny
Copy link
Collaborator

I get the confusion now. With --fix being an option of lint, people expect the normal lint behavior with automatic fixes for correctable rules on top. This would be different if fix was its own command and not an option of lint.

Without a new option, this would be a breaking change, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ideas for improvements of existing features and rules. help Questions or user problems that require more explanation rather than code changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants