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

[Configuration] allow configuring parameterized rules from the configuration file #106

Merged
merged 2 commits into from
Sep 30, 2015

Conversation

jpsim
Copy link
Collaborator

@jpsim jpsim commented Aug 28, 2015

This is awkwardly implemented at the moment because we're starting to duplicate the set of rules in quite a few places, but this works for now.

line_length:
  - 120 # Very Low
  - 140 # Low
  - 160 # Medium
  - 180 # High
  - 200 # Very High

Thoughts @segiddins @keith ?

@keith
Copy link
Collaborator

keith commented Aug 28, 2015

Personally I question the need for the different levels of severity. But I also think projects should have 0 warnings. Regardless 5 of them may be a little more than needed?

I also think that if someone only provides a single number for these it should actually correspond to very high, and work backwards to very low. In our project I would just use very high at 110 for line length for example.

@jpsim
Copy link
Collaborator Author

jpsim commented Aug 28, 2015

Personally I question the need for the different levels of severity.

Same here, to be honest. The original idea was that you may want to warn for some levels, but fail/error for more severe cases. Or you may want to set a minimum severity level you care about depending on the situation? But I think in practice, that complicates usage of SwiftLint more than helping.

Maybe just moving to 2 severity levels (warning & error, which is all that Xcode can present anyway) would be the best way forward?

I understand your intentions in proposing going backwards in the list of parameters, but I think that could get confusing.

@keith
Copy link
Collaborator

keith commented Aug 29, 2015

I think warning vs error is a good distinction. I get that reversing is confusing, I'm not sure what the best way to handle this when someone just adds a single parameter. That is pretty confusion from the users side.

@keith
Copy link
Collaborator

keith commented Aug 29, 2015

Actually if there are only 2 levels I think it makes a lot of sense to be an error unless you add multiple. With 5 reversing is definitely complicated.

@@ -16,6 +17,7 @@ public protocol Rule {

public protocol ParameterizedRule: Rule {
typealias ParameterType
init(parameters: [RuleParameter<ParameterType>])
var parameters: [RuleParameter<ParameterType>] { get }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be removed now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, we still need a way to access the parameters.

@jpsim jpsim force-pushed the jp-configure-parameterized-rules branch from 0fdfc27 to f849df0 Compare August 30, 2015 20:12
@jpsim jpsim force-pushed the jp-configure-parameterized-rules branch from f849df0 to b047d8d Compare August 31, 2015 17:59
@jpsim
Copy link
Collaborator Author

jpsim commented Aug 31, 2015

This has been rebased off master with the changes from #114 so there are now only two severity levels, warning and error.

@jpsim jpsim changed the title [Configuration] allow configuring parameterized rules from the configuration file [WIP] [Configuration] allow configuring parameterized rules from the configuration file Sep 1, 2015
@jpsim
Copy link
Collaborator Author

jpsim commented Sep 1, 2015

Marking this as work-in-progress until I can refactor this a bit to bundle a config file in the SwiftLintFramework.framework bundle and define the default rule parameters in there rather than in code. Thanks @segiddins for the idea.

This will require the ability to "merge" configuration files (framework bundled file + custom file).

@jpsim jpsim force-pushed the jp-configure-parameterized-rules branch from b047d8d to 0581145 Compare September 3, 2015 20:56
@keith keith mentioned this pull request Sep 4, 2015
@jpsim jpsim force-pushed the jp-configure-parameterized-rules branch from 0581145 to 25017c4 Compare September 21, 2015 18:09
@jpsim jpsim changed the title [WIP] [Configuration] allow configuring parameterized rules from the configuration file [Configuration] allow configuring parameterized rules from the configuration file Sep 30, 2015
@jpsim
Copy link
Collaborator Author

jpsim commented Sep 30, 2015

Turns out that refactoring to bundle a config file in the framework and adding the ability to merge config files is a bit out of scope for this PR, and is holding back a release, so I'll merge this now and create a new GitHub issue to track doing this refactoring in the future.

@keith
Copy link
Collaborator

keith commented Sep 30, 2015

Sounds good!

jpsim added a commit that referenced this pull request Sep 30, 2015
[Configuration] allow configuring parameterized rules from the configuration file
@jpsim jpsim merged commit eef5e7c into master Sep 30, 2015
@jpsim jpsim deleted the jp-configure-parameterized-rules branch September 30, 2015 16:51
jpsim added a commit that referenced this pull request Nov 3, 2015
Introduced in #106 and #144 but never documented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants