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

Custom Rules in nested configuration #1815

Closed
2 tasks done
ornithocoder opened this issue Sep 3, 2017 · 7 comments
Closed
2 tasks done

Custom Rules in nested configuration #1815

ornithocoder opened this issue Sep 3, 2017 · 7 comments
Labels
bug Unexpected and reproducible misbehavior.

Comments

@ornithocoder
Copy link
Contributor

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
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 Unexpected and reproducible misbehavior. label Sep 3, 2017
@marcelofabri
Copy link
Collaborator

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

@kenthumphries
Copy link

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

@kenthumphries
Copy link

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
Copy link

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

@aksvenk
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 Add support for merging nested configuration with parent configurations #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
Copy link
Collaborator

jpsim commented Jan 13, 2019

Fixed in #2556

@jpsim jpsim closed this as completed Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected and reproducible misbehavior.
Projects
None yet
Development

No branches or pull requests

6 participants