Linting items for review #98

New Issue

asdf · 2019-11-25T01:36:35Z

asdf commented

2019-11-25 01:36:35 +00:00

As we are closing in on code freeze for 2.0.0, I'm seeing some issues from golangci-lint run (just the default linters enabled):

local/local.go:26:3: ineffectual assignment to `offset` (ineffassign)
		offset := 1
		^
client.go:922:15: Error return value of `saveConfig` is not checked (errcheck)
	go saveConfig()
	             ^
page.go:55:2: SA6003: should range over string, not []rune(string) (staticcheck)
	for _, ch := range []rune(p.RawContent) {
	^
gemini/gemini.go:393:9: S1025: the argument is already a string, there's no need to use fmt.Sprintf (gosimple)
	return fmt.Sprintf("%s", string(bytes.Join(hex, []byte(":"))))
	       ^
gemini/gemini.go:357:7: S1003: should use !strings.Contains(link, "://") instead (gosimple)
			if strings.Index(link, "://") < 0 {
			   ^
client.go:493:18: func `(*client).getCurrentPageRawData` is unused (unused)
func (c *client) getCurrentPageRawData() (string, error) {
                 ^
client.go:486:18: func `(*client).getCurrentPageUrl` is unused (unused)
func (c *client) getCurrentPageUrl() (string, error) {

You can run the entire suite of linters using golangci-lint --enable-all, which returns a very long list. A couple of items I saw that might be of interest:

golangci-lint run --disable-all --enable gosec

golangci-lint run --disable-all --enable godox

What are your thoughts on addressing these, especially in relation to 2.0.0?

As we are closing in on code freeze for 2.0.0, I'm seeing some issues from `golangci-lint run` (just the default linters enabled): ``` local/local.go:26:3: ineffectual assignment to `offset` (ineffassign) offset := 1 ^ client.go:922:15: Error return value of `saveConfig` is not checked (errcheck) go saveConfig() ^ page.go:55:2: SA6003: should range over string, not []rune(string) (staticcheck) for _, ch := range []rune(p.RawContent) { ^ gemini/gemini.go:393:9: S1025: the argument is already a string, there's no need to use fmt.Sprintf (gosimple) return fmt.Sprintf("%s", string(bytes.Join(hex, []byte(":")))) ^ gemini/gemini.go:357:7: S1003: should use !strings.Contains(link, "://") instead (gosimple) if strings.Index(link, "://") < 0 { ^ client.go:493:18: func `(*client).getCurrentPageRawData` is unused (unused) func (c *client) getCurrentPageRawData() (string, error) { ^ client.go:486:18: func `(*client).getCurrentPageUrl` is unused (unused) func (c *client) getCurrentPageUrl() (string, error) { ``` You can run the entire suite of linters using `golangci-lint --enable-all`, which returns a very long list. A couple of items I saw that might be of interest: `golangci-lint run --disable-all --enable gosec` `golangci-lint run --disable-all --enable godox` What are your thoughts on addressing these, especially in relation to 2.0.0?

sloum commented

2019-11-25 03:25:26 +00:00

Is this different than running :goMetaLinter in vim? I have not done so in a quick bit. :-/

Looking at the snapshot you provided:

I'm not sure why this assignment is considered ineffectual... It serves a very clear purpose and is used/referenced further along...
It is a goroutine... to my knowledge a check there would mean we couldnt do it as a goroutine.
Ranging over string produces different results from ranging over []rune(string). We need to range over []rune(string) in order to get character count rather than byte count. Unless I am misunderstanding something.
This seems not logically equivalent. !strings.Contains( ... ) means that if something returns 0 to do what is in the block... but what I want is if something is less than zero. The bang does not work that way.
That function (getCurrentPageRawData) can probably get removed.
That function (getCurrentPageUrl) can probably get removed.

Given that a number of those seem questionable, or at least dont seem to apply to the code as written, I think I am fine with sticking with vim-go's :GoMetaLinter command as the standard for linting. I did just double check that and found a few things leftoever that we had previously discussed and decided to leave.

I'm open to being convinced to go another way, but at this point I feel pretty happy with the codebase.

Is this different than running `:goMetaLinter` in vim? I have not done so in a quick bit. :-/ Looking at the snapshot you provided: - I'm not sure why this assignment is considered ineffectual... It serves a very clear purpose and is used/referenced further along... - It is a goroutine... to my knowledge a check there would mean we couldnt do it as a goroutine. - Ranging over string produces different results from ranging over []rune(string). We need to range over []rune(string) in order to get character count rather than byte count. Unless I am misunderstanding something. - This seems not logically equivalent. `!strings.Contains( ... )` means that if something returns 0 to do what is in the block... but what I want is if something is less than zero. The bang does not work that way. - That function (`getCurrentPageRawData`) can _probably_ get removed. - That function (`getCurrentPageUrl`) can _probably_ get removed. Given that a number of those seem questionable, or at least dont seem to apply to the code as written, I think I am fine with sticking with vim-go's `:GoMetaLinter` command as the standard for linting. I did just double check that and found a few things leftoever that we had previously discussed and decided to leave. I'm open to being convinced to go another way, but at this point I feel pretty happy with the codebase.

asdf commented

2019-11-25 10:16:28 +00:00

vim-go uses golangci-lint but has a specific set of linters enabled when it is run. These results are from the default set of linters used by golangci-lint when run from the command line.

I'm glad these turned out to be mostly invalid. I'm happy to stick with vim-go's :GoMetaLinter, this was something I found when investigating the change from GoMetaLinter to golangci-lint and thought they may be important.

Using :GoCallers on getCurrentPageRawData() and getCurrentPageUrl()` which did not return any results. Searching the text also shows they do not seem to be called. So perhaps they can be removed.

vim-go uses golangci-lint but has a specific set of linters enabled when it is run. These results are from the default set of linters used by golangci-lint when run from the command line. I'm glad these turned out to be mostly invalid. I'm happy to stick with vim-go's `:GoMetaLinter`, this was something I found when investigating the change from GoMetaLinter to golangci-lint and thought they may be important. Using `:GoCallers` on `getCurrentPageRawData() and `getCurrentPageUrl()` which did not return any results. Searching the text also shows they do not seem to be called. So perhaps they can be removed.

sloum commented

2019-11-25 15:35:13 +00:00

Cool. Thank you for clearing up what was being used and how it was being called. I think I am happy to stick with the vim configuration of glangci-lint, at least for now. As to the two methods that are unused, lets ditch them (unless there is a useful place to use them... I'm sure they were written for a reason, but that reason likely shifted away from them over time). I think deleting them should be fine. I'll make a PR later today unless I see you have made one before I get to it.

asdf referenced this issue

2019-11-26 01:31:15 +00:00

Remove unused functions getCurrentPageRawData & getCurrentPageUrl #100

asdf commented

2019-11-26 01:32:06 +00:00

PR #100 is open for this change, no further action is required.

asdf closed this issue

2019-11-26 01:32:07 +00:00

Sign in to join this conversation.