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

Excluded files are being linted #87

Closed
noremac opened this issue Feb 14, 2018 · 33 comments
Closed

Excluded files are being linted #87

noremac opened this issue Feb 14, 2018 · 33 comments
Assignees

Comments

@noremac
Copy link

noremac commented Feb 14, 2018

I have a few directories set up to be excluded by SwiftLint in my .swiftlint.yml file. However, if these files change as part of a pull request, they are still linted by this plugin. Is there a way to disable this, or maybe this is a bug? I've been poking around in the readme and past issues, but could not find a mention of something like this. I apologize if I missed it!

I am using version0.12.1 and my Dangerfile is set up like this:

swiftlint.binary_path = 'Pods/SwiftLint/swiftlint'
swiftlint.config_file = '.swiftlint.yml'
swiftlint.lint_files inline_mode: true

Thanks for the help!

@ashfurrow
Copy link
Owner

Hi there! Really good question, thanks for opening an issue. This is a bug in danger-ruby-swiftlint. I hit a similar problem in danger-rubocop (SwiftLint but for Ruby). However, Rubocop has a handy flag that I used to get around the problem. That flag doesn't exist in SwiftLint, but I have opened a feature request to add one. If the request is turned down, we can still fix our bug here by parsing SwiftLint's config file itself and filtering out files. I'd strongly prefer to not do that, so I'm going to wait and see what they say first.

I'm afraid I can't think of any easy workaround on your end in the mean time. Sorry for the bug – thanks for understanding while we get this sorted out.

@ashfurrow ashfurrow self-assigned this Feb 14, 2018
@noremac
Copy link
Author

noremac commented Feb 15, 2018

Thanks for the quick and thorough follow up! I'd be happy to help review or test anything that comes out of this.

@ashfurrow
Copy link
Owner

I've got an open PR to add this to SwiftLint: realm/SwiftLint#2056

@omirho
Copy link
Collaborator

omirho commented Mar 21, 2018

@ashfurrow Any update on this?

@ashfurrow
Copy link
Owner

The SwiftLint PR has not yet been merged, I’ve asked for an update from them.

@ashfurrow
Copy link
Owner

The PR has just been merged, we'll need to wait for a new version of SwiftLint to be made before moving forward.

@ashfurrow
Copy link
Owner

Still waiting for the next SwiftLint release, I'll check back next weekend.

@heidiproske
Copy link

heidiproske commented Apr 6, 2018

@ashfurrow I've actually been using danger-ruby-swiftlint 0.12.1 quite happily across a number of projects and it has been working well for me (using the same Dangerfile setup as defined in this Issue). Specifically it has been adhering to exclusion request defined in my swiftlint.yml configuration:

excluded: 
- Pods

However, I recently did a bundle update and my danger-swiftlint version was updated as below:

-    danger-swiftlint (0.12.1)
+    danger-swiftlint (0.16.0)

With this update, I'm now finding that I'm getting hundreds of lint warnings for files in my Pods folder 😨.

As a test, I backdated two of my projects to use every danger-ruby-swiftlint version since 0.12.1 and reran my danger checks. My tests are showing that that this bug was specifically introduced in 0.16.0. i.e. if I run any older version 0.15.0, 0.14.0, 0.13.1 then it works well and I don't get the unwanted lint warnings for my exluded Pods folder.

So I'm weirdly not finding the same result as reported in this issue i.e. My excluded files are correctly being excluded with 0.12.1 and all versions after that except for with the latest version 0.16.0 where I'm getting undesired warnings on these excluded files.

As a short-term solution, I've gone ahead and pinned down my danger-ruby-swiftlint so that it will only use 0.15.0 but I'd obviously like to use the latest one.

Curious to hear your thoughts :)

@ashfurrow
Copy link
Owner

Hmmm interesting! Thank you @heidiproske for investigating, I appreciate it and it helps narrow down this problem. There are two possibilities:

  • We changed something in danger-ruby-swiftlint in 0.16.0 that caused things to break, or
  • SwiftLint changed something and that is surfacing as an error here.

I think the first is most likely. This might actually be a separate bug from #87, I'm not sure yet.

I don't have a tonne of time to debug this. I believe @AvdLee is most familiar with this part of the codebase, and the interop with SwiftLint. Do you have time to investigate? The changes are here: 0.15.0...0.16.0

@heidiproske
Copy link

I'll take another look tonight, I've never tried to debug a gem before so it'll be a learning opp!

@ashfurrow
Copy link
Owner

New SwiftLint has been released: https://github.com/realm/SwiftLint/releases/tag/0.25.1

I’ll get on to updating this plugin this week.

@AvdLee
Copy link
Collaborator

AvdLee commented Apr 10, 2018

Yes @ashfurrow, you're right. @heidiproske the only thing introduced in 0.16 apart of extra logging is the support for environment variables (See this pr).

Do you use any environment variables in your config? It might be that they now get parsed correctly, which introduces the issues.

It might also help to link your config file in this issue, so we can help you further.

@ashfurrow
Copy link
Owner

I've opened a PR to use the new --force-excludes flag here: #102 Any contributors up for reviewing it?

@ashfurrow
Copy link
Owner

This is fixed on master, I'll try to get a release done this week. Is someone available to point at the master branch in their Gemfile and test it out?

@noremac
Copy link
Author

noremac commented Apr 24, 2018

I just gave this a shot, here's what I did:

  1. Updated my Gemfile to: gem 'danger-swiftlint', :github => 'ashfurrow/danger-ruby-swiftlint'
  2. bundle update danger-swiftlint
  3. Added a file named blah.swift with the following contents into my Pods folder (which is excluded in my .swiftlint.yml)
let x: Int? = 0
 print(x!)
  1. I then added that same code into a file that I would expect to be linted during a PR.

When the build finished, it reported both warnings instead of the just warning that I would expect from step 4. In this case, the warning is: /tmp/sandbox/workspace/Pods/blah.swift#L2 - Force unwrapping should be avoided.

Is there any other information you'd like me to grab to help diagnose?

@ashfurrow
Copy link
Owner

@noremac thank you – what does your SwiftLint config file look like? And could you confirm the SwiftLint version that CI is running?

@noremac
Copy link
Author

noremac commented Apr 24, 2018

Hey!

We are using SwiftLint 0.25.1 via a CocoaPod, and we are pointing at it like this in our Dangerfile:

swiftlint.binary_path = 'Pods/SwiftLint/swiftlint'

Here's my very lightly obfuscated .swiftlint.yml.

disabled_rules: # rule identifiers to exclude from running
  - function_parameter_count
  - line_length
  - identifier_name
  - cyclomatic_complexity
  - nesting
  - large_tuple
  - unused_closure_parameter
  - for_where
  - notification_center_detachment
  - xctfail_message
  - fallthrough
  - superfluous_disable_command

opt_in_rules: # some rules are only opt-in
  - overridden_super_call
  - prohibited_super_call
  - force_unwrapping
  - empty_count
  - quick_discouraged_focused_test
  - sorted_first_last
  - override_in_extension
  - operator_usage_whitespace
  - operator_whitespace
  - closure_spacing
  - contains_over_first_not_nil
  - first_where
  - joined_default_parameter
  - redundant_optional_initialization
  - unneeded_parentheses_in_closure_argument

excluded: # paths to ignore during linting. Takes precedence over `included`.
  - Carthage
  - Pods
  - Application/Resources/CodeGen
  - Application/Utilities/UI/Web View Controller/ARIntegration/ARCommandErrorCode.swift
  - Frameworks/FusionGraph/Classes/graphql.swift
  - Frameworks/FusionContentLayer/Classes/Utilities/Swift Algorithm Club/Heap/Heap.swift
  - Frameworks/FusionContentLayer/Classes/Utilities/Swift Algorithm Club/Priority Queue/Priority Queue.swift
  - Frameworks/FusionContentLayer/Tests/HeapTests.swift
  - Frameworks/FusionContentLayer/Tests/PriorityQueueTests.swift
  - Vendor

type_body_length:
  - 700 #warning
  - 1000 #error

file_length:
  - 1000 #warning
  - 1200 #error

function_body_length:
  - 125 #warning
  - 200 #error

large_tuple:
  - 4 #warning
  - 5 #error

type_name:
  min_length: 3 # only warning
  max_length: # warning and error
    warning: 50
    error: 50

custom_rules:
  force_https:
    name: "Force HTTPS over HTTP"
    regex: "((?i)http(?!s))"
    match_kinds: string
    message: "HTTPS should be favored over HTTP"
    severity: warning

  explicit_failure_calls:
    name: "Avoid asserting 'false'"
    regex: "((assert|precondition)\\(false)"
    message: "Use assertionFailure() or preconditionFailure() instead."
    severity: warning

  avoid_ipv4: # https://developer.apple.com/news/?id=05042016a
    name: "Avoid hard-coded IPV4 addresses"
    match_kinds: string
    regex: "([01]?[0-9][0-9]?|2[0-4][0-9]|25[0-5])(\\.([01]?[0-9][0-9]?|2[0-4][0-9]|25[0-5])){3}"
    message: "Use IPV6-compatible addresses instead."
    severity: warning

@noremac
Copy link
Author

noremac commented Apr 24, 2018

I also see this log on our build server: Using danger-swiftlint 0.16.0 from git://github.com/ashfurrow/danger-ruby-swiftlint.git (at master@e0227bd) And e0227bd does appear to be the latest at the time of writing this.

@ashfurrow
Copy link
Owner

Ok thanks. That confirms that this issue is still present. Hmm. I'm going to re-open the issue but I'm honestly not sure when I'll be free to look into this further, probably not for another few weeks. If anyone else is available to look into this, I would be grateful.

@noremac could you add verbose: true to your invocation? It could help.

@ashfurrow ashfurrow reopened this Apr 24, 2018
@noremac
Copy link
Author

noremac commented Apr 24, 2018

Trying that now, I'll report back shortly. Thank you!

@AvdLee
Copy link
Collaborator

AvdLee commented Apr 25, 2018

@noremac just to be sure, you didn't set the environment variable anywhere?

ENV['SWIFTLINT_VERSION'] = '0.25.0'

That overrides the used Swiftlint version as seen here.

And as Ash mentioned, verbose would indeed help to see which config is being used and which files are excluded.

@noremac
Copy link
Author

noremac commented Apr 25, 2018

I feel confident that the correct swiftlint version is being used because I initially had forgotten to upgrade it along with the gem, and I started getting errors about the missing --force-excludes option when the plugin ran.

As for the verbose setting, I added swiftlint.verbose = true into my Dangerfile, but I did not see anything new on my build server's logs.

@ashfurrow
Copy link
Owner

@noremac Thanks. For adding verbose logging, the syntax is something like swiftlint.lint_files inline_mode: true, verbose: true.

@AvdLee
Copy link
Collaborator

AvdLee commented Apr 26, 2018

This is the way we use it:

swiftlint.verbose = true
swiftlint.lint_files inline_mode: true

@noremac
Copy link
Author

noremac commented May 10, 2018

Sorry for the delay! Here is the output:

Running: bundle exec danger --fail-on-errors=true
    Using config file: /tmp/sandbox/workspace/.swiftlint.yml
    Swiftlint will be run from /private/tmp/sandbox/workspace
    Swiftlint will exclude the following paths: ["/tmp/sandbox/workspace/Pods", "/tmp/sandbox/workspace/Application/Resources/CodeGen", "/tmp/sandbox/workspace/Application/Utilities/UI/Web View Controller/ARIntegration/ARCommandErrorCode.swift", "/tmp/sandbox/workspace/Frameworks/FusionGraph/Classes/graphql.swift", "/tmp/sandbox/workspace/Frameworks/FusionContentLayer/Classes/Utilities/Swift Algorithm Club/Heap/Heap.swift", "/tmp/sandbox/workspace/Frameworks/FusionContentLayer/Tests/HeapTests.swift", "/tmp/sandbox/workspace/Frameworks/FusionContentLayer/Tests/PriorityQueueTests.swift"]
    Swiftlint includes the following paths: []
    Swiftlint will lint the following files: /private/tmp/sandbox/workspace/Application/App/AppController.swift, /private/tmp/sandbox/workspace/Pods/blah.swift
    linting with options: {:config=>"/tmp/sandbox/workspace/.swiftlint.yml", :reporter=>"json", :quiet=>true, :pwd=>"/private/tmp/sandbox/workspace", :force_exclude=>""}

Looks like this might be on my end. I see /private/tmp vs /tmp in a few places.

@omirho
Copy link
Collaborator

omirho commented May 10, 2018

Yeah.
@noremac Seems like an issue on your end.
As can be seen from the logs, the excluded paths are the ones in your config files, but the linted paths are different.

I'm confused about these 2 lines..

Using config file: /tmp/sandbox/workspace/.swiftlint.yml
Swiftlint will be run from /private/tmp/sandbox/workspace

Please debug and let us know what the issue was.

If you are not able to debug, having your Dangerfile and directory structure would help a lot.

@klaaspieter
Copy link
Contributor

I think I'm seeing a different bug with this behavior, but I'm unsure if this is a bug in this project or SwiftLint itself.

When the force_excluded argument is given I get No lintable files found at path '/PATH/TO/CHANGE/FILE'.

Relevant output:

Using config file: .swiftlint.yml
Swiftlint will be run from /Users/kp/Developer/seed/ios-app
Swiftlint will exclude the following paths: []
Swiftlint includes the following paths: ["/Users/kp/Developer/seed/ios-app/Apps", "/Users/kp/Developer/seed/ios-app/Libraries"]
Swiftlint will lint the following files: /Users/kp/Developer/seed/ios-app/Apps/Seed/Seed/Authentication/AuthenticatedRoute.swift
linting with options: {:config=>".swiftlint.yml", :reporter=>"json", :quiet=>true, :pwd=>"/Users/kp/Developer/seed/ios-app", :force_exclude=>""}
No lintable files found at path '/Users/kp/Developer/seed/ios-app/Apps/Seed/Seed/Authentication/AuthenticatedRoute.swift'
Received from Swiftlint: #<Danger::FileList:0x00007f9dbb3249b0>

Editing the source to remove the force_exclude solves the problem. So does downgrading to 0.16.0.

@ashfurrow
Copy link
Owner

Thanks @klaaspieter – appreciate the details!

I'm in a bit of a bind right now that I don't have the free time available to fix this issue – and I'm not using this tool anymore for my projects, so I can't really justify spending work time on this problem. If someone else is available to investigate and submit a PR to fix this, I would really appreciate it.

Econa77 added a commit to Clipy/Clipy that referenced this issue Jul 3, 2018
ref ashfurrow/danger-ruby-swiftlint#87
force-exclusions options does not work well
Econa77 added a commit to Clipy/Clipy that referenced this issue Jul 3, 2018
* Apply SwiftLint for code checking by danger.systems

* Test for checking danger

* Add danger-swiftlint plugin

* Modify Dangerfile

* Downgrade danger-swiftlint plugin

ref ashfurrow/danger-ruby-swiftlint#87
force-exclusions options does not work well

* Fix SwiftLint warning
@AliSoftware
Copy link
Contributor

AliSoftware commented Oct 16, 2018

I confirm that because of the force_exclude parameter, I have the same error as @klaaspieter

But in fact I think that's a SwiftLint bug. If I manually invoke bundle exec swiftlint --force-exclude --path foo.swift … while foo.swift isn't in my list of excluded: files in my .swiftlint.yml, I still have the SwiftLint message: No lintable files found at path '/Users/[…]/foo.swift'

$ Pods/SwiftLint/swiftlint version
0.27.0

@klaaspieter What I did to workaround the issue in the meantime is that I invoked danger-swiftlint like this in my Dangerfile; HTH

# Note: The argument "--force-exclude" is passed to SwiftLint by the danger-swiftlint plugin
#       But this SwiftLint argument doesn't work properly & has issues.
#       So instead, we re-disable it, and filter the list of files manually :-/
files_to_lint = (git.modified_files - git.deleted_files) + git.added_files
files_to_lint.reject! { |f| f.start_with?('Pods/') }
swiftlint.lint_files(files_to_lint,
  additional_swiftlint_args: '--no-force-exclude'
)

@kevnm67
Copy link

kevnm67 commented Jan 9, 2019

@AliSoftware Your workaround solved the No lintable files found at path issue for me. Thanks!

@nderkach
Copy link

nderkach commented Jun 13, 2019

It seems that the issue still persists with 0.21.1 (SwiftLint 0.33.0). The workaround suggested by @AliSoftware does work with other lint-files parameters like inline_mode:

files_to_lint = (git.modified_files - git.deleted_files) + git.added_files
swiftlint.lint_files(files_to_lint,
  additional_swiftlint_args: '--no-force-exclude',
  inline_mode: true
)

@ashfurrow
Copy link
Owner

Hmm, okay. Seems like this is caused by an underlying problem with the --force-exclude option passed to SwiftLint? I'm the one who implemented that originally, and I'm happy to look into fixing it. Sorry to be obtuse, but this is a really long-lived issue and a really clear report on what the problem is would be helpful. @nderkach @AliSoftware would either of you mind providing steps to reproduce, expected results, and actual results, when running SwiftLint with --force-exclude? I can look into the SwiftLint bug further.

RonnyFenrich pushed a commit to RonnyFenrich/Clipy that referenced this issue Feb 16, 2022
* Apply SwiftLint for code checking by danger.systems

* Test for checking danger

* Add danger-swiftlint plugin

* Modify Dangerfile

* Downgrade danger-swiftlint plugin

ref ashfurrow/danger-ruby-swiftlint#87
force-exclusions options does not work well

* Fix SwiftLint warning
@hlung
Copy link

hlung commented Nov 16, 2022

Does any one notice that using --no-force-exclude gives an error below?

Error: Unknown option '--no-force-exclude'. Did you mean '--force-exclude'?

This stops Dangerfile scripts from running at all...

@noremac noremac closed this as not planned Won't fix, can't repro, duplicate, stale Jan 6, 2024
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

No branches or pull requests

10 participants