Add rule note to output message (#77)

* Add rule note to output message

* rename add_note_to_message to include_note

* Improve code coverage

README.md

@@ -207,8 +207,10 @@ rules:
       - white-list
     alternatives:
       - allowlist
+    note: An optional description why these terms are not inclusive. It can be optionally included in the output message. 
     # options:
     #   word_boundary: false
+    #   include_note: false
 ```
 
 #### Options
@@ -220,6 +222,10 @@ Current options include:
 - `word_boundary` (default: `false`)
   - If `true`, terms will trigger violations when they are surrounded by ASCII word boundaries.
   - If `false`, will trigger violations if the term if found anywhere in the line, regardless if it is within an ASCII word boundary.
+- `include_note` (default: `not set`)
+  - If `true`, the rule note will be included in the output message explaining why this violation is not inclusive
+  - If `false`, the rule note will not be included in the output message
+  - If `not set`, `include_note` in your `woke` config file (ie `.woke.yml`) regulates if the note should be included in the output message (default: `false`). 
 
 #### Disabling Default Rules
 

pkg/config/config.go

@@ -17,6 +17,7 @@ type Config struct {
 	Rules              []*rule.Rule `yaml:"rules"`
 	IgnoreFiles        []string     `yaml:"ignore_files"`
 	SuccessExitMessage *string      `yaml:"success_exit_message"`
+	IncludeNote        bool         `yaml:"include_note"`
 }
 
 // NewConfig returns a new Config
@@ -66,6 +67,8 @@ func (c *Config) inExistingRules(r *rule.Rule) bool {
 }
 
 // ConfigureRules adds the config Rules to DefaultRules
+// Configure RegExps for all rules
+// Configure IncludeNote for all rules
 func (c *Config) ConfigureRules() {
 	for _, r := range rule.DefaultRules {
 		if !c.inExistingRules(r) {
@@ -75,6 +78,7 @@ func (c *Config) ConfigureRules() {
 
 	for _, r := range c.Rules {
 		r.SetRegexp()
+		r.SetIncludeNote(c.IncludeNote)
 	}
 }
 

pkg/config/config_test.go

@@ -105,6 +105,36 @@ func TestNewConfig(t *testing.T) {
 		// check default config message
 		assert.Equal(t, "this is a test", c.GetSuccessExitMessage())
 	})
+
+	t.Run("config-add-note-messaage", func(t *testing.T) {
+		// Test when it is configured to add a note to the output message
+		c, err := NewConfig("testdata/add-note-message.yaml")
+		assert.NoError(t, err)
+
+		// check global IncludeNote
+		assert.Equal(t, true, c.IncludeNote)
+
+		// check IncludeNote is set for rule2
+		assert.Equal(t, true, *c.Rules[1].Options.IncludeNote)
+
+		// check IncludeNote is not overridden for rule1
+		assert.Equal(t, false, *c.Rules[0].Options.IncludeNote)
+	})
+
+	t.Run("config-dont-add-note-messaage", func(t *testing.T) {
+		// Test when it is nott configured to add a note to the output message
+		c, err := NewConfig("testdata/dont-add-note-message.yaml")
+		assert.NoError(t, err)
+
+		// check global IncludeNote
+		assert.Equal(t, false, c.IncludeNote)
+
+		// check IncludeNote is not set for rule2
+		assert.Equal(t, false, *c.Rules[1].Options.IncludeNote)
+
+		// check IncludeNote is not overridden for rule1
+		assert.Equal(t, true, *c.Rules[0].Options.IncludeNote)
+	})
 }
 
 func Test_relative(t *testing.T) {

pkg/config/testdata/add-note-message.yaml

@@ -0,0 +1,20 @@
+rules:
+  - name: rule1
+    terms:
+      - rule1
+    alternatives:
+      - alt-rule1
+    severity: warning
+    options:
+      include_note: false
+  - name: rule2
+    terms:
+      - rule2
+    alternatives:
+      - alt-rule2
+    severity: warning
+
+
+
+success_exit_message: this is a test
+include_note: true

pkg/config/testdata/dont-add-note-message.yaml

@@ -0,0 +1,19 @@
+rules:
+  - name: rule1
+    terms:
+      - rule1
+    alternatives:
+      - alt-rule1
+    severity: warning
+    options:
+      include_note: true
+  - name: rule2
+    terms:
+      - rule2
+    alternatives:
+      - alt-rule2
+    severity: warning
+
+
+
+success_exit_message: this is a test

pkg/parser/parser_test.go

@@ -185,6 +185,31 @@ func parsePathTests(t *testing.T) {
 		})
 		assert.NoError(t, err)
 	})
+
+	t.Run("note is not included in output message", func(t *testing.T) {
+		const TestNote = "TEST NOTE"
+		p := testParser()
+		p.Rules[0].Note = TestNote
+		p.Rules[0].Options.IncludeNote = nil
+		pr := new(testPrinter)
+		p.ParsePaths(pr)
+
+		assert.NotContains(t, pr.results[0].Results[0].Reason(), TestNote)
+	})
+
+	t.Run("note is included in output message", func(t *testing.T) {
+		const TestNote = "TEST NOTE"
+		includeNote := true
+		p := testParser()
+		p.Rules[0].Note = TestNote
+		p.Rules[0].Options.IncludeNote = &includeNote
+		// Test IncludeNote flag doesn't get overridden with SetIncludeNote method
+		p.Rules[0].SetIncludeNote(false)
+		pr := new(testPrinter)
+		p.ParsePaths(pr)
+
+		assert.Contains(t, pr.results[0].Results[0].Reason(), TestNote)
+	})
 }
 
 func TestParser_ParsePaths(t *testing.T) {

pkg/printer/json_test.go

@@ -14,6 +14,6 @@ func TestJSON_Print(t *testing.T) {
 	assert.NoError(t, p.Print(buf, res))
 	got := buf.String()
 
-	expected := `{"Filename":"foo.txt","Results":[{"Rule":{"Name":"whitelist","Terms":["whitelist","white-list","whitelisted","white-listed"],"Alternatives":["allowlist"],"Note":"","Severity":"warning","Options":{"WordBoundary":false}},"Violation":"whitelist","Line":"this whitelist must change","StartPosition":{"Filename":"foo.txt","Offset":0,"Line":1,"Column":6},"EndPosition":{"Filename":"foo.txt","Offset":0,"Line":1,"Column":15},"Reason":"` + "`whitelist`" + ` may be insensitive, use ` + "`allowlist`" + ` instead"}]}` + "\n"
+	expected := `{"Filename":"foo.txt","Results":[{"Rule":{"Name":"whitelist","Terms":["whitelist","white-list","whitelisted","white-listed"],"Alternatives":["allowlist"],"Note":"","Severity":"warning","Options":{"WordBoundary":false,"IncludeNote":null}},"Violation":"whitelist","Line":"this whitelist must change","StartPosition":{"Filename":"foo.txt","Offset":0,"Line":1,"Column":6},"EndPosition":{"Filename":"foo.txt","Offset":0,"Line":1,"Column":15},"Reason":"` + "`whitelist`" + ` may be insensitive, use ` + "`allowlist`" + ` instead"}]}` + "\n"
 	assert.Equal(t, expected, got)
 }

pkg/result/lineresult.go

@@ -63,7 +63,7 @@ func FindResults(r *rule.Rule, filename, text string, line int) (rs []Result) {
 
 // Reason outputs the suggested alternatives for this rule
 func (r LineResult) Reason() string {
-	return r.Rule.Reason(r.Violation)
+	return r.Rule.ReasonWithNote(r.Violation)
 }
 
 func (r LineResult) String() string {

pkg/result/pathresult.go

@@ -16,7 +16,7 @@ type PathResult struct {
 // It is similar to Result.Reason, but makes it clear that the violation is
 // with the file path and not a line in the file
 func (r PathResult) Reason() string {
-	return "Filename violation: " + r.Rule.Reason(r.LineResult.Violation)
+	return "Filename violation: " + r.Rule.ReasonWithNote(r.LineResult.Violation)
 }
 
 // MatchPathRules will match the path against all the rules provided

pkg/rule/default.yaml

@@ -10,6 +10,7 @@
     - allowlist
     - inclusion list
   severity: warning
+  note: "The underlying assumption of the whitelist/blacklist metaphor is that white = good and black = bad. Because colors in and of themselves have no predetermined meaning, any meaning we assign to them is cultural: for example, the color red in many Southeast Asian countries is lucky, and is often associated with events like marriages, whereas the color white carries the same connotations in many European countries. In the case of whitelist/blacklist, the terms originate in the publishing industry – one dominated by the USA and England, two countries which participated in slavery and which grapple with their racist legacies to this day."
 
 - name: blacklist
   terms:
@@ -22,6 +23,7 @@
     - blocklist
     - exclusion list
   severity: warning
+  note: "The underlying assumption of the whitelist/blacklist metaphor is that white = good and black = bad. Because colors in and of themselves have no predetermined meaning, any meaning we assign to them is cultural: for example, the color red in many Southeast Asian countries is lucky, and is often associated with events like marriages, whereas the color white carries the same connotations in many European countries. In the case of whitelist/blacklist, the terms originate in the publishing industry – one dominated by the USA and England, two countries which participated in slavery and which grapple with their racist legacies to this day."
 
 - name: master-slave
   terms:

pkg/rule/options.go

@@ -2,5 +2,6 @@ package rule
 
 // Options are options that can be configured and applied on a per-rule basis
 type Options struct {
-	WordBoundary bool `yaml:"word_boundary"`
+	WordBoundary bool  `yaml:"word_boundary"`
+	IncludeNote  *bool `yaml:"include_note"`
 }

pkg/rule/rule.go

@@ -122,10 +122,17 @@ func (r *Rule) Reason(violation string) string {
 	return reason.String()
 }
 
+func (r *Rule) includeNote() bool {
+	if r.Options.IncludeNote != nil {
+		return *r.Options.IncludeNote
+	}
+	return false
+}
+
 // ReasonWithNote returns a human-readable reason for the rule violation
 // with an additional note, if defined.
 func (r *Rule) ReasonWithNote(violation string) string {
-	if len(r.Note) == 0 {
+	if len(r.Note) == 0 || !r.includeNote() {
 		return r.Reason(violation)
 	}
 	return fmt.Sprintf("%s (%s)", r.Reason(violation), r.Note)
@@ -191,3 +198,14 @@ func removeInlineIgnore(line string) string {
 func (r *Rule) Disabled() bool {
 	return len(r.Terms) == 0
 }
+
+// SetIncludeNote populates IncludeNote attributte in Options
+// Options.IncludeNote is ussed in ReasonWithNote
+// If "include_note" is already defined for the rule in yaml, it will not be overridden
+func (r *Rule) SetIncludeNote(includeNote bool) {
+	if r.Options.IncludeNote != nil {
+		return
+	}
+
+	r.Options.IncludeNote = &includeNote
+}

pkg/rule/rule_test.go

@@ -49,6 +49,7 @@ func TestRule_ReasonWithNote(t *testing.T) {
 	assert.Equal(t, "`rule-1` may be insensitive, use `alt-rule1`, `alt-rule-1` instead", r.ReasonWithNote("rule-1"))
 
 	r.Note = "rule note here"
+	r.SetIncludeNote(true)
 	assert.Equal(t, "`rule-1` may be insensitive, use `alt-rule1`, `alt-rule-1` instead (rule note here)", r.ReasonWithNote("rule-1"))
 }
 
@@ -154,3 +155,15 @@ func Test_removeInlineIgnore(t *testing.T) {
 		})
 	}
 }
+
+func TestRule_IncludeNote(t *testing.T) {
+	r := testRule()
+	includeNote := true
+
+	assert.Equal(t, false, r.includeNote())
+
+	// Test IncludeNote flag doesn't get overridden with SetIncludeNote method
+	r.Options.IncludeNote = &includeNote
+	r.SetIncludeNote(false)
+	assert.Equal(t, true, r.includeNote())
+}