Ignore global variables prefixed with //go:embed

[zbindenren]

This should fix #21.

[leighmcculloch]

Thanks @zbindenren! It looks great. Thanks for adding a test too. I have a few questions (❓) below that are for filling gaps in my own knowledge 😄.

[pavelnikolov]

On a side note, it only makes sense to check for this if the file imports the embed package.

[zbindenren]

On a side note, it only makes sense to check for this if the file imports the embed package.

I didn't know that. I was thinking that with:

//go:embed path/to/file
file []byte

it is not necessary. But it is.

[ldez]

@zbindenren are you still working on this PR?

[leighmcculloch]

Is this PR just waiting on a review and merge? Apologies if this fell off my queue.

[zbindenren]

I am finished, I can make a new PR but it changes the isAllowed signature. But with this change it would better. Shall update the PR?

[bombsimon]

@zbindenren I think you should make the change in this PR, just add a new commit, Would be nice to get this merged and update golangci-lint!

[dackroyd]

This work appears to have stalled? Is assistance needed to get it completed?

[leighmcculloch]

@dackroyd Yup, a lot of the work was done, but I'm sure everyone is dealing with life 😄.

[leighmcculloch]

I've made a few changes to this PR.

• Added a variety of test cases.
• Added support for embed's that aren't grouped inside a var group. A var group is like var ( i1; i2; )
• Added support for embed comments that have an empty line between the comment and the variable. (@bombsimon Your suggestion to use a comment map worked well. @zbindenren I think the reason we weren't able to get it to work before was because the node used to construct the comment map needs to be the ast.File, not the node we're searching for.)
• Won't ignore embeds if the "embed" package is not imported. (@pavelnikolov You mentioned this.)

This PR at commit 700e76b contains all these changes.

@zbindenren @bombsimon @pavelnikolov @ldez You all left comments about this feature. Would any of you be up for taking a last look at this change, and possibly trying it on your codebases to see if there are any edge cases we might be missing?

If nobody sees any issues, I'll merge this and cut a release.

[leighmcculloch]

I pushed one more change, removing the import "embed" check. The Go compiler already errors if you try to use //go:embed in a file without the import, so it is redundant for us to make this checker check for that.

This is also the same reason why the checker doesn't need to handle the case where a go embed comment is placed on a var group. The Go compiler will also error on that.

[bombsimon]

Awesome, thanks for finally taking a stab at this! ⭐

This looks great to me but I can think of a few use cases that might be worth adding tests for (and also ensure working):

/*
This is a long comment
Spanning over multiple lines
*/
//go:embed file.txt
var f string

And the opposite:

//go:embed file.txt
/*
This is a long comment
Spanning over multiple lines
*/
var f string

And I don't know why/if anyone would do this, but this does work as well so if possible maybe should be valid just like we discussed with empty lines between comment and declaration:

/*                
Comment 1         
*/                
//go:embed foo.txt
/*                
Comment 2         
*/                
var s string      

[leighmcculloch]

@bombsimon Nice additional test cases! Added! All pass! 🎉