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

Custom Rules in nested configuration #1815

Closed
ornithocoder opened this Issue Sep 3, 2017 · 7 comments

Comments

Projects
None yet
6 participants
@ornithocoder
Copy link
Contributor

ornithocoder commented Sep 3, 2017

New Issue Checklist

Bug Report

I'm using a custom rule to validate the name of my Quick Specs (I want them to end with Tests, e.g. AppCordinatorTests). For that, I implemented a simple custom rule and placed it in the .swiftlint.yml that I have inside the Tests directory. It didn't work. So I moved the custom rule to the top level .swiftlint.yml and it worked.

Apparently custom rules don't work if placed in nested configurations.

Environment

  • SwiftLint version (run swiftlint version to be sure)? 0.22.0
  • Installation method used (Homebrew, CocoaPods, building from source, etc)? Homebrew
  • Paste your configuration file:
# ...

custom_rules:
  quickspec_class_name:
    name: "Invalid Test Class Name"
    regex: "class\\s+(.*)[^Tests]:\\s+QuickSpec"
    message: "A test class name must end with 'Tests'"
    severity: error
  • Are you using nested configurations? Yes, I am.

  • Which Xcode version are you using (check xcode-select -p)? Xcode 9b6 (App and Command Line)

// This triggers a violation:
class Foo: QuickSpec { }

// Doesn't trigger a violation:
class FooTests: QuickSpec { }

@ornithocoder ornithocoder changed the title Custom Rules in nested directory Custom Rules in nested configuration Sep 3, 2017

@ornithocoder

This comment has been minimized.

Copy link
Contributor Author

ornithocoder commented Sep 3, 2017

To reproduce it, create a file called issue1815.sh and paste the following:

#!/usr/bin/env sh

mkdir Issue1815
cd Issue1815

touch .swiftlint.yml
mkdir Tests
cd Tests

cat > .swiftlint.yml << EOF
custom_rules:
  quickspec_class_name:
    name: "Invalid Test Class Name"
    regex: "class\\\\s+(.*)[^Tests]:\\\\s+QuickSpec"
    message: "A test class name must end with 'Tests'"
    severity: error
EOF

cat > FooTests.swift << EOF
// This triggers a violation:
class Foo: QuickSpec { }

// Doesn't trigger a violation:
class FooTests: QuickSpec { }
EOF

Then run sh issue1815.sh. It will create a sample project that reproduces the problem. Run swiftlint from the top directory (inside Issue1815) and it will not trigger the violation. If you move the rule from Tests/.swiftlint.yml to .swiftlint.yml it triggers the violation.

@marcelofabri marcelofabri added the bug label Sep 3, 2017

@marcelofabri

This comment has been minimized.

Copy link
Collaborator

marcelofabri commented Sep 3, 2017

Could reproduce it, but I needed to change cat > .swiftlint.yaml << EOF to cat > .swiftlint.yml << EOF in the script 😉

@kenthumphries

This comment has been minimized.

Copy link

kenthumphries commented Sep 11, 2017

This issue appears for me when configuring a standard (non custom) rule in a nested configuration also.

@kenthumphries

This comment has been minimized.

Copy link

kenthumphries commented Sep 11, 2017

To reproduce it, create a file called issue1815-StandardRule.sh and paste the following:

#!/usr/bin/env sh

mkdir Issue1815-StandardRule
cd Issue1815-StandardRule

touch .swiftlint.yml
mkdir Tests
cd Tests

cat > .swiftlint.yml << EOF
line_length:
  warning: 50
  error: 70

EOF

cat > FooTests.swift << EOF
// This triggers a violation:
class FooTests: VeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongQuickSpec { }

// Doesn't trigger a violation:
class Foo: QuickSpec { }
EOF

Then run sh issue1815-StandardRule.sh. It will create a sample project that reproduces the problem. Run swiftlint from the top directory (inside Issue1815-StandardRule) and it will not trigger the violation. If you move the rule from Tests/.swiftlint.yml to .swiftlint.yml it triggers the violation.

@michaelloo

This comment has been minimized.

Copy link

michaelloo commented Sep 27, 2017

Hi there, I'm also experiencing the same issue. When can we expect a fix?

@aksvenk

This comment has been minimized.

Copy link

aksvenk commented Jun 19, 2018

The reason that doesn't work is because of the way swiftlint currently handles merges. It is not a true 'merge' of the rules and it's values but a 'union' of rules by identifiers.

In @kenthumphries case:

  • The nested config in the Spec directory is attempted to be merged to the root config
  • The root config is blank and hence contains all default rules with their default severity levels
  • The root config rules are unioned (Set operation) with the nested config
  • Since the uniqueness of the rule is determined by it's identifier, the nested rule is completely ignored.
  • All the root config rules 'win' in this case and the error and warning threshold overrides in the nested config are completely ignored
  • This is contrary to what #1674 says

If you want to restore the original behaviour of a "nested config replacing the root config", please take a look at my open PR #2245
This is a backward compatible since the behaviour can be disabled using a commandline option.

@marcelofabri I feel this has regressed somewhere along the way. Let me know if you want me to raise a bug.

Reference
The merging strategy can be found here:

private func mergingRules(with configuration: Configuration) -> [Rule] {

@jpsim

This comment has been minimized.

Copy link
Collaborator

jpsim commented Jan 13, 2019

Fixed in #2556

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment