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

Add configurations to Rules.md #1653

Closed
marcelofabri opened this issue Jul 2, 2017 · 6 comments
Closed

Add configurations to Rules.md #1653

marcelofabri opened this issue Jul 2, 2017 · 6 comments
Labels
enhancement Ideas for improvements of existing features and rules. wontfix Issues that became stale and were auto-closed by a bot.

Comments

@marcelofabri
Copy link
Collaborator

Now that #1107 was merged, we can work on improvements:

List rules configuration. In order to do this right, we'd need to change a bit the RuleConfiguration protocol to provide more information about the parameters.

@marcelofabri marcelofabri added the enhancement Ideas for improvements of existing features and rules. label Jul 2, 2017
@marcelofabri
Copy link
Collaborator Author

Here's a summary of the changes I want to do to enable this:

  • Make all rules to take a dictionary. This will have the side effect of removing support to implicit configurations (Deprecate/remove implicit configurations #1475), but I think this is better in the longer term.
  • Add a var parameters: ParameterDefinition { get } on RuleConfiguration. This will be used for documentation and initing the variables in a RuleConfiguration.

This is a simple example for TrailingCommaConfiguration (removing the severity configuration for demonstration purposes):

protocol YamlLoadable {}

extension Bool: YamlLoadable {}

protocol ParameterDefinition {
    var key: String { get }
    var defaultValueDescription: String { get }
    var description: String { get }
}

struct Parameter<T: YamlLoadable>: ParameterDefinition {
    let key: String
    let `default`: T
    let description: String
    let defaultValueDescription: String

    func parse(from configuration: [String: Any]) throws -> T {
        if let value = configuration[key] as? T {
            return value
        } else if configuration[key] != nil {
            throw ConfigurationError.unknownConfiguration
        }

        return `default`
    }
}

public struct TrailingCommaConfiguration: RuleConfiguration, Equatable {
    private(set) var mandatoryComma: Bool

    private static let mandatoryCommaParam = Parameter(key: "mandatory_comma",
                                                       default: false,
                                                       description: "Is Mandatory comma",
                                                       defaultValueDescription: "false")

    static let parameters: [ParameterDefinition] = [mandatoryCommaParam]

    public init(mandatoryComma: Bool = mandatoryCommaParam.default) {
        self.mandatoryComma = mandatoryComma
    }

    public mutating func apply(configuration: [String: Any]) throws {
        mandatoryComma = try type(of: self).mandatoryCommaParam.parse(from: configuration)
    }
}

I'd love to hear more thoughts about this since it'd be quite a breaking change. // @jpsim

@norio-nomura
Copy link
Collaborator

I have a WIP branch that makes Yams compliant with Swift Encoders.
I think that it is worth having the Configuration conform to Codable.

@marcelofabri
Copy link
Collaborator Author

I have a WIP branch with the changes proposed here. This will make possible to generate a documentation file like this.

There are a lot of work yet before we merge it:

  • Migrate all the remaining rule configurations (the ones that are missing are the ones that have nested configurations).
  • Update all the parameters descriptions.
  • Think whether this is a good approach, as this removes the ability to use arrays or single values as configurations.
  • Update configurations to expose properties that provide the value from a parameter (e.g. severity for a severityParameter). This way, consumers (rules) don't need to know about parameters.
  • Think whether we could use Sourcery to eliminate some duplication.
  • With this approach, it would be easy to validate that a rule configuration only contains valid keys.
  • Figure out how all of this play together with Swift Encoders.

@donald-m-ritter
Copy link
Contributor

Could there be a temporary non-breaking solution of adding a new variable to RuleDescription that the documentation generator can pull in and add?

public struct RuleDescription: Equatable {
    public let identifier: String
    public let name: String
    public let description: String
    public let kind: RuleKind
    public let nonTriggeringExamples: [String]
    public let triggeringExamples: [String]
    public let corrections: [String: String]
    public let deprecatedAliases: Set<String>
    public let configurationExamples: [String]

    public var consoleDescription: String { return "\(name) (\(identifier)): \(description)" }

    public var allIdentifiers: [String] {
        return Array(deprecatedAliases) + [identifier]
    }

    public init(identifier: String, name: String, description: String, kind: RuleKind,
                nonTriggeringExamples: [String] = [], triggeringExamples: [String] = [],
                corrections: [String: String] = [:],
                deprecatedAliases: Set<String> = [],
                configurationExamples: [String] = []) {
        self.identifier = identifier
        self.name = name
        self.description = description
        self.kind = kind
        self.nonTriggeringExamples = nonTriggeringExamples
        self.triggeringExamples = triggeringExamples
        self.corrections = corrections
        self.deprecatedAliases = deprecatedAliases
        self.configurationExamples = configurationExamples
    }
}

Then the documentation generator would add the output of that similar to the triggering and non-triggering examples.

Configuration Examples
leading_whitespace:
  severity: warning
file_header:
  required_pattern: ^\/\/\n\/\/  (.+)\n\/\/  (.+)\n\/\/\n\/\/  Created by (.+) on (\d{1,2}\/\d{1,2}\/\d{1,2}).\n\/\/  Copyright © \d{4} Realm. All rights reserved.\n\/\/$
  severity: warning

@Robuske
Copy link

Robuske commented Aug 22, 2018

Sooo... Any update on this?

@stale
Copy link

stale bot commented Nov 8, 2020

This issue has been automatically marked as stale because it has not had any recent activity. Please comment to prevent this issue from being closed. Thank you for your contributions!

@stale stale bot added the wontfix Issues that became stale and were auto-closed by a bot. label Nov 8, 2020
@stale stale bot closed this as completed Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ideas for improvements of existing features and rules. wontfix Issues that became stale and were auto-closed by a bot.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants