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 rule note to output message #77

Merged
merged 3 commits into from
May 22, 2021

Conversation

eedorenko
Copy link
Contributor

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
The rule definition has "Note" attribute which is supposed to contain explanation why the rule terms are concidered not inclusive. It would be beneficial for education purposes if the linter could (optionally) provide in its output not only suggestions with alternatives, but the reason/explanation as well. So the author could understand why they should care about this violation.

What is the current behavior? (You can also link to an open issue here)
Currently the "Note" is not shown anywhere. There is a method ReasonWithNote which is supposed to provide the desired behavior, but it's not used.

What is the new behavior (if this is a feature change)?

  • .woke.yaml configuration file may contain add_note_to_message (bool) attribute, so if it's true (false by default) the output message will contain the Note for all rules unless it's specified otherwise explicitly at the rule level
  • The rule definition Options may contain add_note_to_message (bool) attribute, which dictates if the Note should be included in the output message for this specific rule.

Does this PR introduce a breaking change? (What changes might users need to make due to this PR?)
No

Other information:

@eedorenko eedorenko force-pushed the eedorenko/add-note-to-message branch from d19ff01 to 9d8f8fe Compare May 20, 2021 22:24
@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #77 (45c326e) into main (2e9f79d) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head 45c326e differs from pull request most recent head 6865593. Consider uploading reports for the commit 6865593 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   95.52%   95.58%   +0.06%     
==========================================
  Files          21       21              
  Lines         469      476       +7     
==========================================
+ Hits          448      455       +7     
  Misses         12       12              
  Partials        9        9              
Impacted Files Coverage Δ
pkg/config/config.go 100.00% <100.00%> (ø)
pkg/result/lineresult.go 100.00% <100.00%> (ø)
pkg/result/pathresult.go 100.00% <100.00%> (ø)
pkg/rule/rule.go 91.17% <100.00%> (+0.85%) ⬆️
pkg/parser/parser.go 100.00% <0.00%> (ø)
pkg/parser/violations.go 95.00% <0.00%> (+0.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e9f79d...6865593. Read the comment docs.

@eedorenko eedorenko marked this pull request as draft May 21, 2021 20:36
@eedorenko eedorenko force-pushed the eedorenko/add-note-to-message branch from 45c326e to 6865593 Compare May 21, 2021 22:49
@eedorenko eedorenko marked this pull request as ready for review May 21, 2021 22:55
Copy link
Member

@caitlinelfring caitlinelfring left a comment

Choose a reason for hiding this comment

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

Thanks for expanding the test coverage and for making this contribution!

@caitlinelfring caitlinelfring merged commit 4e1a9a0 into get-woke:main May 22, 2021
@caitlinelfring
Copy link
Member

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.

None yet

2 participants