ashfurrow / danger-ruby-swiftlint

A Danger plugin for SwiftLint.
https://rubygems.org/gems/danger-swiftlint
MIT License
203 stars 81 forks source link

Excluded files are being linted #87

Closed noremac closed 9 months ago

noremac commented 6 years ago

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 commented 6 years ago

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.

noremac commented 6 years ago

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

ashfurrow commented 6 years ago

I've got an open PR to add this to SwiftLint: https://github.com/realm/SwiftLint/pull/2056

omirho commented 6 years ago

@ashfurrow Any update on this?

ashfurrow commented 6 years ago

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

ashfurrow commented 6 years ago

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

ashfurrow commented 6 years ago

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

heidiproske commented 6 years ago

@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 commented 6 years ago

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

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: https://github.com/ashfurrow/danger-ruby-swiftlint/compare/0.15.0...0.16.0

heidiproske commented 6 years ago

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

ashfurrow commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

I've opened a PR to use the new --force-excludes flag here: https://github.com/ashfurrow/danger-ruby-swiftlint/pull/102 Any contributors up for reviewing it?

ashfurrow commented 6 years ago

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 commented 6 years ago

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!)
  4. 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 commented 6 years ago

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

noremac commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

noremac commented 6 years ago

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

AvdLee commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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

AvdLee commented 6 years ago

This is the way we use it:

swiftlint.verbose = true
swiftlint.lint_files inline_mode: true
noremac commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

AliSoftware commented 6 years ago

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 commented 5 years ago

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

nderkach commented 5 years ago

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 commented 5 years ago

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.

hlung commented 1 year ago

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...