Hierarchical Linting

[aarondaub]

@jpsim – I think this is a good way to handle the linting. I don't currently like how I build the child linters from lines, it's very imperative. I'd LOVE suggestions on improvements there.

This'll work with an arbitrary depth of "linter context comments" (see discussion on #4).

This addresses #1 and #23

TODO:

• Allow linter context beginning and ending to be less strict (e.g. different whitespace)
• Perhaps move the LinterRegion to be a property on LinterContext and then pass the file to each sublinter without modification
• Add unit tests
• Fix issues with linter so that children don't Lint using rules that don't apply to them and we don't get false negatives for Rules that require the whole file because we're linting in chunks
• Add ability for a child linter to invalidate a style violation if it occurred in a region that the child linter was responsible and that child linter's LinterContext had that rule disabled.

[aarondaub]

@jpsim we should probably close #12 in favour of this :) (this is a superset of that)

[aarondaub]

I think it would be a good idea to have the LinterContext contain the file region and then we can have an array of contexts not regions and contexts. A LinterContext should query its region of the file to find out what rules to enable and disable.

Thoughts?

[aarondaub]

Ah, @jpsim, I found a pretty fundamental issue with this approach. If each Linter only has a subregion of the File then we run into issues when with rules like FileLengthRule, TrailingNewLineRule, and NestingRule.

A potential solution: a way to "overlint" (by which I mean have the parent linter run against it's entire file and all of the children run against their subregions) and then filter out violations that occurred because of Rules that were disabled in the region in which they occurred. Children should also only lint with the DELTA of rules so they don't report rules the parent reported AND don't report rules that can only happen at the top level (e.g. TrailingNewLineRule/FileLengthRule).

[jpsim]

First of all, thank you very much for your keen interest in contributing and all the work you're putting in, it's great to see.

But please, before you continue to invest more of your valuable time, please share your design ideas so we can truly think through what problem we're aiming to solve and which approaches would work best.

As we briefly discussed yesterday in person, I'm not convinced adding configurability and configured regions needs to be this complex, e.g. generate new linters per-region.

Here's how I'm considering configurability could be implemented (very rough concept):

typealias RuleIdentifier = String

public struct Configuration {
    public let enabledRuleIDs: Set<RuleIdentifier>
    public let disabledRuleIDs: Set<RuleIdentifier>
    public static let defaultConfiguration = Configuration()

    public init(enabledRuleIDs: Set<RuleIdentifier> = Set(),
        disabledRuleIDs: Set<RuleIdentifier> = Set()) {
        self.enabledRuleIDs = enabledRuleIDs
        self.disabledRuleIDs = disabledRuleIDs
    }

    public init?(file: File) {
        // TODO: Parse YAML configuration file
        return nil
    }

    public func applyCommand(command: ConfigurationCommand) -> Configuration {
        // TODO: Apply command
        return self
    }

    public func generateConfigurationsForFileRegions(regions: [FileRegion]) -> [Configuration] {
        // TODO: Generate configurations, starting from the current configuration as root and
        //       applying the commands
        return []
    }
}

extension File {
    public var configuredRegions: [FileRegion] {
        // TODO: Recurse through file to identify all configured regions
        return []
    }
}

public struct FileRegion {
    public let file: File
    public let range: NSRange
    public let commands: [ConfigurationCommand]

    public init(file: File, range: NSRange, commands: [ConfigurationCommand]) {
        self.file = file
        self.range = range
        self.commands = commands
    }
}

public enum ConfigurationCommand {
    case EnableRule(ruleIdentifier: RuleIdentifier)
    case DisableRule(ruleIdentifier: RuleIdentifier)
}

I may work on this a bit during my flights today...

[aarondaub]

I definitely like the above approach and will reach out regarding design decisions in the future. Do you have an idea of how generateConfigurationsForFile regions would be used with only one Linter? I like this way of handling Configuration and will bounce some ideas around my head but I'll hold off on committing anything before you have had some more time flesh this out.

[jpsim]

Linting the delta of rules won't work, because there may not always be a way to represent that "delta".

I'd rather have a dictionary of mutually exclusive Dictionary<FileRegion, Configuration>s so that each region only ever has a single flattened Configuration.

[jpsim]

Also, I'd highly recommend that you make an effort to follow the project's existing style more closely (4 spaces per indent, newlines at ends of files, let > var where possible, etc.). It will make reviewing your PRs much easier 😄.

[aarondaub]

Ah, I hadn't thought of flattening the Configurations/LinterContexts. That sounds promising but I don't know if it would be easy to do or not. As far as the delta goes: preventing rules from being validated against if they were already done so in the parent was easy enough, but I haven't experimented with filtering out false negatives. Are you foreseeing an issue with that?

Your code snippet was very interesting though. A big improvement to this PR would be to make the linters property on Linter return Configurations then move that logic to Configuration. We could also split the region separation code into an extension on File, I had considered that but I didn't know if the File should have specific knowledge about how Configurations are delimited.

And yeah, I need to update my Xcode indentation settings and be more diligent about vars that aren't needed after refactors :)

[jpsim]

@aarondaub feel free to continue working on this functionality if you'd like. I ended up pushing up a proof-of-concept for configurable regions here, which you can use for inspiration: https://github.com/realm/SwiftLint/compare/jp-configuration

[aarondaub]

@jpsim cool – I'll spend a bit of time this evening. I'll play around with your branch and see what's missing.

[jpsim]

In that branch, I'm going more the route of explicit commands rather than the stack-based approach discussed in #4 (comment) just because parsing it becomes much simpler.

[aarondaub]

How about I clean up this PR to use much of the same implementation as the other branch but keep the stack approach? Then we can pick between PRs based on what approach we decide to go with. If you think a decision could be made without seeing both implementations I can just build off of the non-stack approach. Thoughts?

[jpsim]

If you can get the stack approach to work, without adding much more complexity, I'd be open to it, but I get the impression that it'll get hairy pretty quickly.

[aarondaub]

I'll play around and let you know how hairy it gets 😄

[aarondaub]

@jpsim just a heads up that I'm still playing with this on my fork for the moment: aarondaub#3. I'll open up a PR if things go smoothly.

[jpsim]

@aarondaub if you're not actively working on this PR, please close it.