Add ability to ignore faillint problems #11

nnutter · 2020-03-07T19:36:38Z

Linter directive is based on staticcheck.

For: #6

fatih · 2020-03-08T04:42:54Z

README.md

+For example,
+
+```go
+//lint:file-ignore faillint This file should be ignored by faillint.


Does it have to be on top of the package keyword or does it not matter where we put this line? I think mentioning this in the readme is important to make it clear.

I’ll add test coverage and update docs accordingly. I believe postfix comment will work.

Postfix did not work as it was but it has been updated so that it does.

fatih · 2020-03-08T04:52:59Z

faillint/faillint.go

+	return false
+}
+
+func commentHasIgnoreDirective(c *ast.Comment) bool {


Better name: hasLineIgnore. But I'm not sure we need this function at all. I think it's better if we just inline them. There is now too much indirections and we jump quite around.

I think the indirection and jumping around has been addressed; there are now only three new functions.

fatih · 2020-03-08T04:57:40Z

faillint/faillint.go

+	return false
+}
+
+func commentGroupsHaveIgnoreDirective(gs []*ast.CommentGroup) bool {


commentsIgnored I think is a shorter name, let's use that.

fatih · 2020-03-08T05:13:15Z

faillint/faillint.go

+		}
+	}
+	return false
+}


I think both fileIsIgnored and usageIsIgnored can be combined into a single funtion, such as:

func IsIgnored(cm *ast.CommentGroup, option string) bool { if cm == nil { return false } for _, comment := range cm.List { cmd, args := parseDirective(comment.Text) if cmd == option && len(args) > 0 && args[0] == "faillint" { return true } } return false }

and then use it like:

isIgnored(file.Doc, "file-ignore)

or

isIgnored(spec.Doc, "ignore")

This also means we can get rid of commentHasIgnoreDirective

As I switched to this style I felt like the function should not have ignore in the name because it could technically be more broad and because it stuttered. I did shorten the names but because of this deviated from your suggestions. Please, of course, let me know what you think of the names once you see it in its new form.

fatih · 2020-03-08T05:19:35Z

Thank you @nnutter. Added add some initial comments so you can go over them.

fatih · 2020-03-08T05:23:32Z

Also now that I think, I wonder if we should use faillint:ignore reason and faillint:file-ignore rason syntax. I'm not sure using statichecks's syntax make sense. People would assume it belongs to staticcheck even if it's not.

Linter directive is based on staticcheck.

fatih · 2020-03-09T05:10:55Z

README.md

+
+#### File-based linter directives
+
+File-based linter directives can be applied to ignore `faillint` problems in a whole file.  The format is,


Can you add a comment that the ignore directive has to be before the package keyword? Or is there any other case that would also work?

Added clarification; in 3986017. If you want it anywhere in the file that can be done but it's probably better to keep it limited to the package docs.

fatih · 2020-03-09T05:12:46Z

faillint/faillint.go

+const (
+	ignore     = "ignore"
+	fileignore = "file-ignore"
+)


Can you move them to the top of the file? Also I think documenting them and naming them in a more descriptive way would be better. Such as: ignoreKeyE and fileIgnoreKey

Refactored now; in 08c1050.

fatih · 2020-03-09T05:14:35Z

README.md

+### Ignoring problems
+
+If you want to ignore a problem reported by `faillint` you can add a linter directive.
+The format of a directive is based on the format other linters, such as `staticcheck`, use and there are two types: line-based and file-based.


I think this line is not applicable anymore and can be removed.

Removed now; in 1c0756d.

fatih · 2020-03-09T05:17:32Z

faillint/faillint.go

+	s = strings.TrimPrefix(s, "//faillint:")
+	fields := strings.Split(s, " ")
+	return fields[0]
+}


I think we should check for the reason as well. And reason should be MUST have. Most of the time people add these lines and later if someone reads it they don't have an idea why it's added. Hence we should make sure it's required. I think staticcheck does also require reason.

Checked now; in c11afeb.

I also added a check for out-of-place file-ignore options; in 01ca523.

In both cases adding a test was not possible in the current framework because adding a // want comment negates the test conditions. I wanted to check with you before trying anything else. Do you want to create a ResultType and return something from run()? Maybe a struct with a slice of analysis.Diagnostic? If so do you want all the existing tests converted to use that? Of course, any other ideas? Maybe I'm overlooking something simple?

Manual testing,

$ go run . -paths errors ./faillint/testdata/src/n /Users/nnutter/src/github.com/nnutter/faillint/faillint/testdata/src/n/n.go:4:2: missing reason on faillint directive exit status 3 (master)faillint 17:44:59 1 [1181] github.com/nnutter/faillint $ go run . -paths errors ./faillint/testdata/src/o /Users/nnutter/src/github.com/nnutter/faillint/faillint/testdata/src/o/o.go:4:2: file-ignore option on faillint directive must be in package docs /Users/nnutter/src/github.com/nnutter/faillint/faillint/testdata/src/o/o.go:5:2: package "errors" shouldn't be imported exit status 3

I think we should have a TestParseDirective that tests the parseDirective function. And then you can check for various parse directives and I think that would be sufficient for our needs.

I merged our two ideas together. =)

Thanks, this is much better 👍

…taticchecks' linter directives

- Rename - Move - Comment

dominikh · 2020-03-10T09:23:11Z

Also now that I think, I wonder if we should use faillint:ignore reason and faillint:file-ignore rason syntax. I'm not sure using statichecks's syntax make sense. People would assume it belongs to staticcheck even if it's not.

FWIW, I designed staticcheck's syntax in the hope that other projects would adopt it, too. It's why staticcheck doesn't flag unknown check names in directives, and why I chose a prefix as generic as lint:, as opposed to, say, sc:.

fatih

@nnutter thanks a lot for all the work. This looks great right now, I added couple of comments for further improvements. I know this took longer than anticipated, but once merged it'll be maintained as long as this project is active, so I want to make sure things are right from the beginning

One last second, after talking to @dominikh, seems like using //lint, just like staticcheck, would be better in the long term. I know you did initially and I hope it's not a big issue changing it.

Once we wrap it all, we're good to go.

fatih · 2020-03-12T13:24:51Z

README.md

+//faillint:file-ignore reason
+```
+
+This must be placed in the `package` docs and, conventionally, these comments should be placed near the top of the file.


I think people might get confused on what package docs means here. Maybe writing it simpler would be better, such as This must be placed above the package keyword, near the top of the file. What do you think?

Problem was it did have to be attached to the package comment/docs. However, I agree that's confusing and probably not really correct/ideal. I'll loosen it to search all comments in the file and then the phrase can be, "This may be placed anywhere in the file but conventionally it should be placed at or near the top of the file."

Done in 868a3ea.

fatih · 2020-03-12T13:26:19Z

faillint/faillint.go

+	s = strings.TrimPrefix(s, "//faillint:")
+	fields := strings.Split(s, " ")
+	return fields[0]
+}


Thanks, this is much better 👍

fatih · 2020-03-12T13:28:40Z

faillint/faillint_test.go

+				out:     false,
+				message: fmt.Sprintf(unexpectedFileIgnoreTemplate, "file-ignore"),
+			},
+		},


I think it would be nice to have also cases with reason and are valid so we can see it works for those as well. Right now seems like we only check for failures but not for successes.

We were covering success in the "integration tests" but it's trivial to add valid cases here and I will do so.

Done in 33d8aa8.

nnutter · 2020-03-13T01:06:26Z

One last second, after talking to @dominikh, seems like using //lint, just like staticcheck, would be better in the long term. I know you did initially and I hope it's not a big issue changing it.

Done in 1151d6a.

nnutter · 2020-03-13T01:08:58Z

I know this took longer than anticipated, but once merged it'll be maintained as long as this project is active, so I want to make sure things are right from the beginning

I totally understand. From my perspective I appreciate your response time and that we are getting an iteration done something like every 24 hours.

fatih reviewed Mar 8, 2020

View reviewed changes

Add ability to ignore faillint problems

c0c2416

Linter directive is based on staticcheck.

nnutter force-pushed the master branch from 943e8b1 to c0c2416 Compare March 9, 2020 00:05

nnutter requested a review from fatih March 9, 2020 00:10

fatih reviewed Mar 9, 2020

View reviewed changes

nnutter added 5 commits March 9, 2020 17:00

Remove reference to staticcheck now that it's more loosely based on s…

1c0756d

…taticchecks' linter directives

Clarify file-ignore placement

3986017

Refactor ignore/fileignore constants

08c1050

- Rename - Move - Comment

Report missing reasons

c11afeb

Report out of place file-ignore option

01ca523

nnutter requested a review from fatih March 9, 2020 23:03

Add test coverage for hasDirective

22b4caf

fatih reviewed Mar 12, 2020

View reviewed changes

nnutter added 5 commits March 12, 2020 19:11

Add valid case coverage to TestHasDirective unit test

33d8aa8

Add test case for variation of package comment

c312b7f

Loosen restrictions on the placement of the file-ignore directive

868a3ea

Remove code which can no longer execute

59d5005

Switch to staticcheck style directives

1151d6a

nnutter requested a review from fatih March 13, 2020 01:06

fatih approved these changes Mar 14, 2020

View reviewed changes

fatih merged commit 3992f98 into fatih:master Mar 14, 2020

fatih mentioned this pull request Mar 14, 2020

Comment imports to tolerate exceptions #6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to ignore faillint problems #11

Add ability to ignore faillint problems #11

nnutter commented Mar 7, 2020

fatih Mar 8, 2020

nnutter Mar 8, 2020

nnutter Mar 9, 2020

fatih Mar 8, 2020

nnutter Mar 9, 2020

fatih Mar 8, 2020

fatih Mar 8, 2020

nnutter Mar 9, 2020

fatih commented Mar 8, 2020

fatih commented Mar 8, 2020

fatih Mar 9, 2020

nnutter Mar 9, 2020

fatih Mar 9, 2020

nnutter Mar 9, 2020

fatih Mar 9, 2020

nnutter Mar 9, 2020 •

edited

Loading

fatih Mar 9, 2020

nnutter Mar 9, 2020

nnutter Mar 9, 2020

nnutter Mar 9, 2020

fatih Mar 11, 2020

nnutter Mar 12, 2020

fatih Mar 12, 2020

dominikh commented Mar 10, 2020

fatih left a comment

fatih Mar 12, 2020

nnutter Mar 13, 2020

nnutter Mar 13, 2020

fatih Mar 12, 2020

fatih Mar 12, 2020

nnutter Mar 13, 2020

nnutter Mar 13, 2020

nnutter commented Mar 13, 2020

nnutter commented Mar 13, 2020


		#### File-based linter directives

		File-based linter directives can be applied to ignore `faillint` problems in a whole file. The format is,

Add ability to ignore faillint problems #11

Add ability to ignore faillint problems #11

Conversation

nnutter commented Mar 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatih commented Mar 8, 2020

fatih commented Mar 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnutter Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikh commented Mar 10, 2020

fatih left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnutter commented Mar 13, 2020

nnutter commented Mar 13, 2020

nnutter Mar 9, 2020 •

edited

Loading