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
Fix: Store rule severity in messages, rather than pulling from the config on-demand #985
Conversation
@@ -81,7 +81,7 @@ function printResults(config) { | |||
console.error("Could not find formatter '%s'.", config.format); | |||
return false; | |||
} |
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.
They no longer need the config, the only thing they were using it for was to obtain rule severity.
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 might be true now, however, there are custom formatters out there in the wild that will break if we just remove it. We need to do a removal in two steps: first we deprecate its usage and then we actually remove it.
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.
Got it. Added 'config' parameter back. What's your procedure for deprecations, anything I should add for that or will it just go in the changelog? Should this be "Breaking: " instead of "Fix: "?
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.
It's technically not breaking at this point, so I'd stick with "Fix:". Just
put "deprecates config for formatters" somewhere in there.
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.
In what? A comment in the code? Description on the commit? Both?
Apparently Github expects you to be adding notes below the changes, not above them. Oh well. |
Can you add "; deprecates passing full config to Formatters" in your commit message? |
Done. Not sure why it isn't updating here. |
…sing full config to Formatters
Proooobably because that was the wrong branch and commit. NOW it's done. |
Thanks! |
Fix: Store rule severity in messages, rather than pulling from the config on-demand
Fixes #984.
This stores the rule severity on the message objects created when a rule generates a report. This ensures that the severity is always what the rule gets at that instant, regardless of anything that's affected the config up to that point. It also removes all dependency on the config from the formatters, they now depend only on the messages.