- Notifications
You must be signed in to change notification settings - Fork8
Allow rules string as a configuration alternative#19
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
8120b86 to7a2eda6Comparefixtures/rules/config.json Outdated
| "enabled": true, | ||
| "channel": "beta", | ||
| "config": { | ||
| "rules": "java-basic,java-design" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a reason why this isn't an array (instead of a string with a comma-separated list)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because I am stupid I guess... =/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ha, no way! 😛
Just a suggestion to simplify parsing. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually we just forward the entire thing, I think that was the reason I didn't bother to split, but I makes more sense from ayml perspective to have it as an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@toddmazierski Adjusted.
| @Test | ||
| public void acceptRulesSimpleNames() { | ||
| def config = new Config([configFile: "/usr/src/app/fixtures/rules/config.json", codeFolder: "/usr/src/app/fixtures/rules"]) | ||
| assertEquals "java-basic,java-design", config.ruleSet() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this assertion be adjusted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's the argument for the command line, should still be a single string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, my mistake.
Shouldconfig.mix.json still contain a comma-separated string, though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does not impact, but it should be adjusted. 👍
toddmazierski left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🚢
Uh oh!
There was an error while loading.Please reload this page.