Dictionary Parameterization of rules #314

scottrhoyt · 2016-01-04T06:06:35Z

Dictionary Parameterization

Closes #303

Here is my idea on how to implement dictionary parameterization for rules. Sorry it is such a beast of a change (and hard to review), but it was necessary IMO to accomplish the task. There are some pretty big changes in here, so I understand if we need to refine this further or even reject this course.

Merge Notes

I merged my recursive configuration branch into this branch before it was integrated into master. In addition to that I have merged master several times to keep up with the other changes. Somehow this resulted in the recursive config commits being duplicated, though the diff's still look correct. This could be a result of the combination of how I merged and the cherry picking that was done to do the final integration.

Overview

This was my thought process as I developed this:

With the increased complexity in rule configuration, it didn't make sense to have Configuration store the knowledge on how to configure each rule anymore. This should be encapsulated in the Rule itself.
A new protocol, ConfigurableRule, was created to facilitate this.
When encapsulating the configuration in ConfigurableRule, I didn't want to create a dependency on Yaml, I chose instead for ConfigurableRule to have a failable initializer taking an AnyObject instead. AnyObject was chosen over the alternative, [String: AnyObject], to support the trivial config cases (e.g. line_length: 100).
I wrote Yaml extensions to flatten the Yaml.Dictionarys into [String: AnyObject]s
I then refactored Configuration to extract Yaml parsing to YamlParser
I also didn't think it made sense anymore to have Configuration store the master rule list, so I extracted this to the global masterRuleList using a rather trivial RuleList container I created.
I wrote protocol extensions for ParameterizedRule to make all legacy parameterized rules automatically conform to ConfigurableRule
Configuration now constructs it's rule array by iterating through a RuleList (defaulting to masterRuleList). If a rule is a ConfigurableRule and a configuration exists for this rule, the rule will be instantiated by passing it's configuration to init?(config:). If this fails, the rule is not a ConfigurableRule, or a configuration does not exist, the Rule is instantiated by calling it's default init.
I added the init() requirement to the Rule protocol to support the initialization of Rules with their default values.
I added the trivial case init() { } to all non-ConfigurableRules to support this.
I wrote unit tests for most of the above.
I adapted VariableNameMinLengthRule as an example of how to create a ConfigurableRule. The new implementation supports the same syntax as before, but now also supports configuration via warning and error parameters as well as a new excluded array parameter to exclude certain variable names from the check (e.g. id). Closes Special case id/exceptions for NameFormatViolation #231.
```
variable_name_min_length:
  warning: 3
  error: 4
  excluded:
    - id
```

Thoughts

I'm not crazy about the AnyObject parameter for configuration. I feel like it's fighting the strong type system a bit. But on the other hand, it seems like a necessary evil to support the use cases we would like. Also, this follows in the well-tread footsteps of serializing objects, and it makes it relatively easy to support any serialization technique for configurations (e.g. json, xml, or plist). The plist option is particularly nice because it would support a tighter integration of SwiftLint into Xcode.
Because we store Rules in a [Rule] for Configuration and I needed to implement equality testing (isEqualTo and Equatable) for Configuration to support unit tests, I needed a way of dealing with two ConfigurableRules being compared while being downcasted to Rules. This result in a somewhat ugly change to the implementation of Rule.isEqualTo that tests for upcasting to ConfigurableRules. This feels like fighting the type system, but I couldn't come up with a way around it. However, this is now a potential trap if more rule protocols are added.
There appears to be some sort of bug in how SwiftXPC overrides comparing two [String]s that results in two empty [String]s not being equal. I had to therefore test for equality in a different way in VariableNameMinLengthRule.
This is a beginning, but we can further this work by writing additional protocols and methods to support common configuration schemes to ease new ConfigurableRule creation. We can also further restrict configuration to remove the AnyObject requirement or create a new type to facilitate this.
Another project I have contributed to, ObjectMapper, can potentially be used to add some nice syntactical sugar to the syntax of ConfigurableRules.
I originally wrote the Yaml flattening as extensions, but it might make more sense to put those in YamlParser.

scottrhoyt · 2016-01-04T06:57:34Z

Also, if we go this route, ParameterizedRule can be deprecated or removed outright. It already wasn't being used much at all since all conforming rules were referenced directly. Every ParameterizedRule can be rewritten as a ConfigurableRule, I just didn't take the time to do that except for VariableNewMinLengthRule.

jpsim · 2016-01-04T21:47:41Z

Thanks for the PR, it will take some time to review 😬

scottrhoyt · 2016-01-04T22:39:01Z

No problem. Just post any questions you have about implementation details or design decisions, and I'll be happy to discuss.

scottrhoyt · 2016-01-07T20:03:27Z

Hey @jpsim. I was wondering if you had any thoughts on this yet? I just don't want to let it go too long because of merge conflicts. If you are ready to take another look, I can clean up whatever conflicts have arisen.

jpsim · 2016-01-07T20:47:22Z

Source/SwiftLintFramework/Extensions/Yaml+SwiftLint.swift

+        return nil
+    }
+
+    var flatArray: [AnyObject]? {


One liner: var flatArray: [AnyObject]? { return array?.map { $0.flatValue } }

jpsim · 2016-01-07T21:45:00Z

@scottrhoyt I really like this. It makes a lot more sense than our current approach. I like the way you built it too.

I've commented a lot of nitpicky stuff as line notes, but taking a step back and looking at the big picture, there's just one thing that jumps out at me for now: can we consolidate the ConfigurableRule and ParameterizedRule protocols? Is seems like we should just need one of these, no?

Otherwise, once you've had a chance to rebase against master and address my feedback, I'll take another look.

scottrhoyt · 2016-01-07T21:53:54Z

Thanks, I'm glad you like it. I think it's a good direction as well. I fixed most of the suggestions and asked for follow up on a couple.

Yes, there is now no need for ParamterizedRule and ConfigurableRule. ParameterizedRule was left as is as a demonstration of the new approach, and also because I didn't want to rewrite all the old ParameterizedRules until I was sure we wanted to go this direction. But as I mentioned above, if we go this route, ParameterizedRule can be deprecated or removed outright--and probably should be removed--before merging.

Once I get that clarification on the couple of outstanding edits, I will rebase against master.

jpsim · 2016-01-07T21:58:03Z

Source/SwiftLintFramework/Rules/VariableNameMinLengthRule.swift

            }
            return []
        } ?? []
    }
+
+    public func isEqualTo(rule: ConfigurableRule) -> Bool {
+        if let rule = rule as? VariableNameMinLengthRule {


good place for a guard here

jpsim · 2016-01-07T22:03:21Z

Yes, there is now no need for ParamterizedRule and ConfigurableRule. ParameterizedRule was left as is as a demonstration of the new approach, and also because I didn't want to rewrite all the old ParameterizedRules until I was sure we wanted to go this direction. But as I mentioned above, if we go this route, ParameterizedRule can be deprecated or removed outright--and probably should be removed--before merging.

Yup, let's remove ParameterizedRule altogether then!

scottrhoyt · 2016-01-07T22:19:38Z

Most of this is cleaned up now. My thinking on removing ParameterizedRules is to initially just move the protocol extension methods into the rule classes themselves and keep the existing initialization implementation. Then we can either wait until more configuration options are added to each rule (which will necessitate changing how init works), or open a separate branch for removing any redundant configuration? Alternatively I can embark upon the conversion now, but that might delay this merge by a couple of days due to my current work load.

scottrhoyt · 2016-01-07T22:21:44Z

Also, with regards to the rebase, I must confess I haven't rebased much, but I'm familiar with the concept and the intended results of cleaning up this commit history. However, I was pretty sure rebasing after pushing to a remote was a no no. Is that not the case? Or should we use a work around like creating a fresh branch to do the rebase and then pushing to github with that, creating a new PR from there, and deleting this branch?

jpsim · 2016-01-07T22:24:04Z

I don't want to take up more of your time, so how about you rebase this to address the merge conflicts and I can do a final review pass as-is before we embark on deprecating ParameterizedRule.

Also, with regards to the rebase, I must confess I haven't rebased much, but I'm familiar with the concept and the intended results of cleaning up this commit history. However, I was pretty sure rebasing after pushing to a remote was a no no. Is that not the case? Or should we use a work around like creating a fresh branch to do the rebase and then pushing to github with that, creating a new PR from there, and deleting this branch?

Some projects discourage rebasing because it does in a sense rewrite history, but I value legible commit history in SwiftLint enough to prefer rebasing over verbose merge noise.

To rebase, first make sure that your local master branch is up to date with this remote, then from your branch you can run git rebase master and use git mergetool to address merge conflicts as they occur.

scottrhoyt · 2016-01-07T22:32:03Z

Great. I'll get that done later today. Something to put in the backlog after this merge will be a new system for documenting rules: so that we can provide documentation via web and command line for the new expanded configuration options. I suppose that will be contingent on what the standards are for creating ConfigurableRules.

jpsim · 2016-01-07T22:34:35Z

Great. I'll get that done later today. Something to put in the backlog after this merge will be a new system for documenting rules: so that we can provide documentation via web and command line for the new expanded configuration options. I suppose that will be contingent on what the standards are for creating ConfigurableRules.

This is one downside to ConfigurableRule's untyped approach rather than ParameterizedRule, which was type-safe.

jpsim · 2016-01-09T00:24:32Z

@scottrhoyt let me know if you're too busy to get to this, I don't mind taking care of the rebase.

…fault implementation for conformance.

jpsim · 2016-01-11T20:06:45Z

Source/SwiftLintFramework/Protocols/Rule.swift

+
+    public func isEqualTo(rule: ConfigurableRule) -> Bool {
+        if let rule = rule as? Self {
+            return self.parameters == rule.parameters


can omit self here probably?

…eMinLengthRule.init?(config:).

scottrhoyt · 2016-01-11T21:58:18Z

@jpsim Incorporated latest feedback and updated CHANGELOG and CONTRIBUTING. Let me know if there is anything else.

jpsim · 2016-01-11T22:00:19Z

This looks ok to me, but it's a large change, so I'd love to get some feedback from @realm/swiftlint thoughts?

nicolai86 · 2016-01-12T07:51:57Z

I don't like that rule configuration parsing is mixed with the validation and correcting of rules. Maybe splitting these concerns would make sense down the line.
E.g. A rule taking a configuration struct, and a configuration parser needs to be declared together with the rule in the masterRule list, which encapsulates the configuration parsing.

However it's a good step to making the rules better configurable! 👍

jpsim · 2016-01-12T19:32:42Z

Cool, this looks like it's good to merge to me. @nicolai86 feel free to decouple that in a PR, or even just file an issue detailing what you think should be done so it doesn't slip through the cracks.

Dictionary Parameterization of rules

jpsim · 2016-01-12T19:42:41Z

Great job @scottrhoyt! I know this was a lot of work, and especially dealing with all my nitpicks, but it turned out great 🎉

scottrhoyt · 2016-01-12T20:28:57Z

@nicolai86 I completely agree with you on that and would love to see some more separation of responsibility as well as perhaps bringing back some type safety to configurations. I think this can be part of the work of standardizing how rules can be configured, how to handle documenting rule configurations, and providing the boilerplate for common rule configs like warning and error levels. I think we can probably go a lot further in extracting more functionality out of Configuration as well.

On the flip side, I do think this is a necessary first step to open the door to more advanced configurations. I also think it is a step forward, single responsibility-wise, from the relatively monolithic Configuration object that previously parsed YAML, stored the master rule list, configured rules, and does the file system traversals. Also, as a replacement for ParameterizedRule, I like that ConfigurableRule now has benefit used as a protocol where ParameterizedRule wasn't referenced as a protocol but just provided a standard template.

@jpsim no problem! Thanks for your feedback and allowing me to contribute in a big way to a great project.

jakecraige · 2016-01-12T23:31:23Z

This is great. 👍 Thanks for all the work @scottrhoyt

scottrhoyt · 2016-01-13T00:29:08Z

No problem!

On Jan 12, 2016, at 3:31 PM, Jake Craige notifications@github.com wrote:

This is great. Thanks for all the work @scottrhoyt

—
Reply to this email directly or view it on GitHub.

norio-nomura · 2016-01-13T00:54:52Z

Great! 🎉

jpsim reviewed Jan 7, 2016
View reviewed changes

scottrhoyt added 6 commits January 11, 2016 11:21

Fix style issues.

35c92ff

Removed unnecessary setUp override.

dd243ed

Changed VariableNameMinLengthRule.isEqualTo to use a guard.

23265f0

Fixed access level issues.

46838b1

Added [Int] support for VariableNameMinLengthRule configuration.

8d6f1ad

Made ParameterizedRule protocol inherit from ConfigurableRule with de…

55fa348

…fault implementation for conformance.

scottrhoyt force-pushed the sh-dictionary-parameters branch from 0d9c03a to 55fa348 Compare January 11, 2016 19:24

jpsim reviewed Jan 11, 2016
View reviewed changes

scottrhoyt added 3 commits January 11, 2016 13:10

Extracted arrayOf to Array extension and used to simplify VariableNam…

6473715

…eMinLengthRule.init?(config:).

Updated CHANGELOG and CONTRIBUTING.

97c6b5c

Removed redundant self.

47f4889

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

Merge pull request #314 from realm/sh-dictionary-parameters

9d138ad

Dictionary Parameterization of rules

jpsim merged commit 9d138ad into master Jan 12, 2016

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

minor tweaks from #314

7268d37

jpsim deleted the sh-dictionary-parameters branch January 12, 2016 19:42

jpsim mentioned this pull request Jan 13, 2016

Add "Missing Docs" rule #201

Merged

scottrhoyt restored the sh-dictionary-parameters branch January 14, 2016 03:51

scottrhoyt deleted the sh-dictionary-parameters branch January 14, 2016 03:51

scottrhoyt mentioned this pull request Jan 21, 2016

Configuration Structs #391

Merged

3 tasks

jpsim mentioned this pull request Feb 2, 2016

On 0.7.2 "swiftlint rules" does not output all data #470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dictionary Parameterization of rules #314

Dictionary Parameterization of rules #314

scottrhoyt commented Jan 4, 2016

scottrhoyt commented Jan 4, 2016

jpsim commented Jan 4, 2016

scottrhoyt commented Jan 4, 2016

scottrhoyt commented Jan 7, 2016

jpsim Jan 7, 2016

jpsim commented Jan 7, 2016

scottrhoyt commented Jan 7, 2016

jpsim Jan 7, 2016

jpsim commented Jan 7, 2016

scottrhoyt commented Jan 7, 2016

scottrhoyt commented Jan 7, 2016

jpsim commented Jan 7, 2016

scottrhoyt commented Jan 7, 2016

jpsim commented Jan 7, 2016

jpsim commented Jan 9, 2016

jpsim Jan 11, 2016

scottrhoyt commented Jan 11, 2016

jpsim commented Jan 11, 2016

nicolai86 commented Jan 12, 2016

jpsim commented Jan 12, 2016

jpsim commented Jan 12, 2016

scottrhoyt commented Jan 12, 2016

jakecraige commented Jan 12, 2016

scottrhoyt commented Jan 13, 2016

norio-nomura commented Jan 13, 2016

Dictionary Parameterization of rules #314

Dictionary Parameterization of rules #314

Conversation

scottrhoyt commented Jan 4, 2016

Dictionary Parameterization

Merge Notes

Overview

Thoughts

scottrhoyt commented Jan 4, 2016

jpsim commented Jan 4, 2016

scottrhoyt commented Jan 4, 2016

scottrhoyt commented Jan 7, 2016

jpsim Jan 7, 2016

Choose a reason for hiding this comment

jpsim commented Jan 7, 2016

scottrhoyt commented Jan 7, 2016

jpsim Jan 7, 2016

Choose a reason for hiding this comment

jpsim commented Jan 7, 2016

scottrhoyt commented Jan 7, 2016

scottrhoyt commented Jan 7, 2016

jpsim commented Jan 7, 2016

scottrhoyt commented Jan 7, 2016

jpsim commented Jan 7, 2016

jpsim commented Jan 9, 2016

jpsim Jan 11, 2016

Choose a reason for hiding this comment

scottrhoyt commented Jan 11, 2016

jpsim commented Jan 11, 2016

nicolai86 commented Jan 12, 2016

jpsim commented Jan 12, 2016

jpsim commented Jan 12, 2016

scottrhoyt commented Jan 12, 2016

jakecraige commented Jan 12, 2016

scottrhoyt commented Jan 13, 2016

norio-nomura commented Jan 13, 2016