Configuration Structs #391

scottrhoyt · 2016-01-21T03:19:07Z

Configuration Structs

This PR expands upon the work in #314 and #354 by providing struct-based configurations for the common patterns and providing default conformance to ConfigurableRule via these structures.

Key Benefits

All Rules now have some sort of configuration available.
- Basic rules are now configurable in their violation severity. (SeverityConfig)
- Rules with multiple severity levels are configurable in one or both levels. (SeverityLevelsConfig)
- Name evaluation rules are configurable in their min_length and max_length (both warning and error) as well as with excluded names. (NameConfig)
Rule configurations can be partly or fully customized.
Performance has been increased by ~2% (vs master) by folding VariableNameMinLengthRule and VariableNameMaxLengthRule into VariableNameRule.
Test coverage has been increased.

Key Drawbacks

This introduces some additional verbosity because ConfigProviderRules need to access their configuration via config and it's properties. This can be somewhat mitigated with more convenience computed properties.

Example YAML

The following configuration is now possible.

force_cast: Warning      # supports severity config case-insensitive
todo:
  severity: error        # severity can be configure key/value as well
line_length: 120         # only warning configured
file_length:
  warning: 500
  error: 1200
function_body_length:    # warning and error via array still supported
  - 40
  - 60
type_name:
  min_length: 4          # only warning
  max_length:            # warning and error
    warning: 40
    error: 50
  excluded: iPhone       # excluded via string
variable_name:
  min_length:            # only min_length
    error: 4             # only error
  excluded:              # excluded via string array
    - id
    - URL
    - GlobalAPIKey

Open Issues Addressed

Closes Make TypeNameRule conform to ConfigurableRule #388
Closes Configure severity of rules #371
Closes Ability to customize severity / color of individual rules #130
Workaround for Adding --strict does not log or display linter violations as errors #268 (via severity)
Workaround for Global Let should allow capitalization of first charater #259 (via excluded)
Workaround for Issues with the Variable and Type Name Rule #191 (via excluded)

Design Decisions

Used structs for RuleConfigs to encapsulate configuration via Dictionarys and also promote maximum reusability (e.g. NameConfig uses two SeverityLevelsConfigs).
Left ConfigurableRule in place as a catch-all for non-standard configs (e.g. MissingDocsRule)
Used RuleConfig.setConfig(config:) throws instead of a failable initializer for ingesting configuration dictionaries. This is to support the coexistence of default values and configured values so that partial configurations can be provided via the YAML.
Switched ConfigurableRule.init?(config:) to ConfigurableRule.init(config:) throwsto gracefully propograte these ConfigurationErrors.

TODO

Update README
Update CHANGELOG
Update CONTRIBUTING

/cc @realm/swiftlint

norio-nomura · 2016-01-21T04:20:57Z

Source/SwiftLintFramework/Protocols/RuleConfig.swift

+public protocol RuleConfig {
+    mutating func setConfig(config: AnyObject) throws
+    func isEqualTo(ruleConfig: RuleConfig) -> Bool
+}


It can use:

extension RuleConfig where Self: Equatable { public func isEqualTo(ruleConfig: RuleConfig) -> Bool { if let ruleConfig = ruleConfig as? Self { return self == ruleConfig } return false } }

norio-nomura · 2016-01-21T04:33:44Z

Great!

scottrhoyt · 2016-01-21T04:55:55Z

This might be a good time to add ViolationSeverity.Note so that it is an option for severity configuration. @jpsim , if I just add the enum case, will that be enough to support it for XcodeReporter?

UPDATE

I played around with notes, and it appears that while Xcode recognizes them as a special case, it does note display them anywhere in the GUI.

jpsim · 2016-01-21T19:28:39Z

Awesome! At first glance, I like this direction, but I might not be able to review in depth for a few days. I'm certainly happy to see this address so many issues users have faced regarding configuration!

jpsim · 2016-01-23T02:10:40Z

Source/SwiftLintFramework/Models/ConfigurationError.swift

+
+import Foundation
+
+public enum ConfigurationError: ErrorType {


Why introduce this if there's only one member, and it's never checked? ConfigurableRuleType.init(config:) is only ever used with try?, which type-erases the error in a sense.

This was something I thought about for a bit. Basically what was the best way to support configuration structures failing and provide optional configuration values functionality. To use the failable initializers approach that was previously used in ConfigurableRule would have required each rule to have its own RuleConfig type because you could only declare the default values in the structure. The second approach was to split initialization with default values and configuration with optional values into init and setConfig. Since we needed a way to indicate failure in setConfig, we could either throw or return some sort of success/failure flag (e.g. Bool, Result, etc.). Throwing felt the most Swifty to me, and I needed something to throw, so that's how this came to be.

There's certainly a case to be made that if we use this approach, we should introduce some finer-grain errors, possibly merge this with YamlParsingError, and probably change Configuration.init? to Configuration.init() throws. That's all possible in this PR, but in my head I was scoping it to the rework that needs to be done on logging and errors.

For the time being, I made this marginally more useful by not squashing the error in Configuration.rulesFromDict, though nothing is really being done with it. Let me know if you want to combine this with YamlParserError. I like that we have one less error type with that approach. But I don't like that a ConfigProviderRule would even know about something that knows about the Yaml subsystem.

jpsim · 2016-01-23T02:20:44Z

This is pretty awesome! I'm happy with almost all of this, other than where noted, and the remaining checklist items from the original PR comment.

It shouldn't be too much work to rebase this on master for now, since it's only conflicting with #377 AFAICT, but #397 will probably cause much more rebase work, so we should probably merge this one first.

scottrhoyt · 2016-01-23T18:13:33Z

Great. I'll make those changes today so we can get this in.

scottrhoyt · 2016-01-23T20:50:29Z

@jpsim take a look at this. Only outstanding issue is the use ConfigurationError. Let me know what you think about README and CONTRIBUTING. Is the provided info on customizing through .swiftlint.yml and contributing a ConfigProviderRule clear enough?

scottrhoyt · 2016-01-23T21:01:07Z

FWIW, unit test took ~4 seconds before the rebase and ~16 seconds after on my computer. Probably related to the work with not counting comments and whitespace lines, but the tests don't feel like unit tests anymore.

Update

Just seeing #401 addresses this now.

jpsim · 2016-01-23T23:15:49Z

CONTRIBUTING.md

+* If none of the provided `RuleConfig`s are applicable, you can create one
+  specifically for your rule.
+
+See [`ForceTryRule`](https://github.com/realm/SwiftLint/blob/master/Source/SwiftLintFramework/Rules/ForceCastRule.swift)


Mentions ForceTryRule but links to ForceCastRule.

Good catch!

jpsim · 2016-01-23T23:24:10Z

Only outstanding issue is the use ConfigurationError.

Actually your explanation makes sense: throws propagates errors, and better to throw an error in a domain under our control than a random NSError. 👍

Let me know what you think about README and CONTRIBUTING. Is the provided info on customizing through .swiftlint.yml and contributing a ConfigProviderRule clear enough?

Very useful and informative 👍 Thanks for adding the opt-in segment too!

Other than my last few comments, big 👍 on this!

…uded.

…uatable.

… SeverityConfig.

… Configuration.rulesFromDict.

scottrhoyt · 2016-01-24T02:21:15Z

@jpsim I think we are all good here now.

jpsim · 2016-01-24T02:35:51Z

👍 💯 kudos. Can you make sure to close all the issues resolved by this?

Configuration Structs

scottrhoyt · 2016-01-24T02:38:02Z

No problem. The 3 that are 100% addressed have been closed. What do you want to do with the 3 that this provided a solid workaround for?

jpsim · 2016-01-24T03:04:47Z

I think this PR is the best way to address #191, despite not being exactly what the user wanted, so I'd close that one as resolved. The other 2 can remain open.

norio-nomura · 2016-01-24T06:20:30Z

Wow! 🎉

tamarnachmany · 2016-01-24T20:01:21Z

🍕

derpoliuk · 2016-07-06T10:19:52Z

Thanks for PR!

Is there anyway to set severity for all rules? I want to make sure that build fails if any SwiftLint rule is violated.

ikesyo · 2016-07-06T10:25:51Z

@derpoliuk What you want would be the --strict option of lint command.

[--strict]
    fail on warnings

derpoliuk · 2016-07-06T10:31:11Z

@ikesyo thanks for very quick answer!

I know about --strict option (should've addressed it in my first comment), but it doesn't provide pretty output (as discussed in #268). Though it's a working solution.

scottrhoyt changed the title ~~Configuration Structure~~ Configuration Structs Jan 21, 2016

norio-nomura reviewed Jan 21, 2016
View reviewed changes

scottrhoyt mentioned this pull request Jan 21, 2016

Generic regex rules #123

Closed

jpsim reviewed Jan 23, 2016
View reviewed changes

scottrhoyt force-pushed the sh-configuration-structs branch from c921a34 to 9a77974 Compare January 23, 2016 20:46

jpsim reviewed Jan 23, 2016
View reviewed changes

jpsim mentioned this pull request Jan 23, 2016

Prepare codebase for Swift Package Manager support #398

Merged

scottrhoyt added 10 commits January 23, 2016 17:42

Added ConfigurationProviderRule

d300eec

Added ViolationLevelConfiguration.

21abbd3

Convert ConfigurableRule to throwing init instead of failing.

ccbcb53

Add MinMaxConfiguration.

1a2bb1b

Fix spacing.

619a345

Added associated type for ConfigurationProviderRule.

d2ec3fb

TypeNameRule conforms to ConfigurationProviderRule.

6522add

Fixed tests for throwing ConfigurableRule init.

b92c241

Fixed TypeNameRule to match previous definition.

4ad287d

Delete unused ViolationLevelConfiguration protocol.

6d38573

scottrhoyt added 14 commits January 23, 2016 17:42

Switch VariableNameRule to use a guard for early exit if name is excl…

123d06e

…uded.

Provide default implementation for RuleConfig.isEqualTo when it is Eq…

6132d5d

…uatable.

Use guard in NameConfig.setConfig for better semantics.

42a6e9f

Switch SeverityConfig.setConfig to guard for better semantics.

9f20fc2

Remove unnecessary Swift namespace for min and max calls.

95a7352

Removed ViolationSeverity extension. Moved into a private function of…

e2265cf

… SeverityConfig.

Make ConfigurationError marginally more useful by not squashing it in…

6107822

… Configuration.rulesFromDict.

Reset VariableNameRule to instantiating Location at the call site.

b145bf1

Update README.

87c5342

Update CHANGELOG

d1574bb

Update CONTRIBUTING

53218ec

Fixed typo in CONTRIBUTING

6718de4

Reset VariableNameRule to if check on return as opposed to guard.

44ff8ef

Rebased. Fixed new conflicts.

12aba5f

scottrhoyt force-pushed the sh-configuration-structs branch from 4d9f367 to 12aba5f Compare January 24, 2016 01:48

Switch to if/early return for VariableNameRule.

75f2068

jpsim added a commit that referenced this pull request Jan 24, 2016

Merge pull request #391 from realm/sh-configuration-structs

e9897c1

Configuration Structs

jpsim merged commit e9897c1 into master Jan 24, 2016

jpsim deleted the sh-configuration-structs branch January 24, 2016 02:35

This was referenced Jan 25, 2016

Issues with the Variable and Type Name Rule #191

Closed

Adding --strict does not log or display linter violations as errors #268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration Structs #391

Configuration Structs #391

scottrhoyt commented Jan 21, 2016

norio-nomura Jan 21, 2016

scottrhoyt Jan 21, 2016

scottrhoyt Jan 21, 2016

norio-nomura Jan 21, 2016

norio-nomura commented Jan 21, 2016

scottrhoyt commented Jan 21, 2016

jpsim commented Jan 21, 2016

jpsim Jan 23, 2016

scottrhoyt Jan 23, 2016

scottrhoyt Jan 23, 2016

jpsim commented Jan 23, 2016

scottrhoyt commented Jan 23, 2016

scottrhoyt commented Jan 23, 2016

scottrhoyt commented Jan 23, 2016

jpsim Jan 23, 2016

scottrhoyt Jan 24, 2016

jpsim commented Jan 23, 2016

scottrhoyt commented Jan 24, 2016

jpsim commented Jan 24, 2016

scottrhoyt commented Jan 24, 2016

jpsim commented Jan 24, 2016

norio-nomura commented Jan 24, 2016

tamarnachmany commented Jan 24, 2016

derpoliuk commented Jul 6, 2016

ikesyo commented Jul 6, 2016

derpoliuk commented Jul 6, 2016


		import Foundation

		public enum ConfigurationError: ErrorType {

Configuration Structs #391

Configuration Structs #391

Conversation

scottrhoyt commented Jan 21, 2016

Configuration Structs

Key Benefits

Key Drawbacks

Example YAML

Open Issues Addressed

Design Decisions

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norio-nomura commented Jan 21, 2016

scottrhoyt commented Jan 21, 2016

jpsim commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented Jan 23, 2016

scottrhoyt commented Jan 23, 2016

scottrhoyt commented Jan 23, 2016

scottrhoyt commented Jan 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented Jan 23, 2016

scottrhoyt commented Jan 24, 2016

jpsim commented Jan 24, 2016

scottrhoyt commented Jan 24, 2016

jpsim commented Jan 24, 2016

norio-nomura commented Jan 24, 2016

tamarnachmany commented Jan 24, 2016

derpoliuk commented Jul 6, 2016

ikesyo commented Jul 6, 2016

derpoliuk commented Jul 6, 2016