-
Notifications
You must be signed in to change notification settings - Fork 12
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
linter #18
linter #18
Conversation
f, err := os.Open(execFile) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to open the executable file %s: %w", execFile, err) | ||
} | ||
defer f.Close() | ||
defer func(f *os.File) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add G307
into global excludes here https://github.com/vxcontrol/soldr/blob/master/.golangci.yml#L68
Such wrappers everywhere in codebase is redundant and non-profitable. I will not add this comment in every place with the same pattern, please consider this comment for all files.
@@ -2,6 +2,7 @@ package app | |||
|
|||
import ( | |||
"github.com/oklog/run" | |||
"github.com/sirupsen/logrus" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use direct logrus
imports, because we have app level router configured using app config https://github.com/vxcontrol/soldr/blob/master/cmd/api/main.go#L153
and package internal/log
which should be use for dependency.
Note that not all parts of code uses it now, but it will in future.
@@ -13,7 +14,10 @@ func NewAppGroup() *AppGroup { | |||
} | |||
|
|||
func (a *AppGroup) Run() { | |||
a.runGroup.Run() | |||
err := a.runGroup.Run() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add logging here and in other places just for suppressing linter messages. Logging is a part of public application output. All these new log messages for errors is not useful information about how app runs.
Let's return err
if this is possible, or ignore like _ = a.runGroup.Run()
, or ignore with comment for linter, and make there places visible and marked for refactoring. In time all such places will be substituted with proper error handling.
sLoadModulesSQL string = ` | ||
SELECT | ||
m.id | ||
, IFNULL(g.hash, '') AS group_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just ignore lll
for this const. really weird formatting here, with ,
in the beginning of row
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This technique is necessary in order not to forget the comma when adding. Similar to how Go puts a comma at the end of a line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, writing in one line makes the constant absolutely unreadable for the developer.
vm vm.VM | ||
agentID string | ||
version string | ||
//connProtocolVersion string - unused field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's delete that
continue | ||
} | ||
} | ||
} | ||
} | ||
|
||
func removeAll(path string) { | ||
e := os.RemoveAll(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why this is not working https://github.com/vxcontrol/soldr/blob/master/.golangci.yml#L77
This should be ignored by linter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the linter ignores this line. However, it is strange that in the original version he did not do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the duplicate code into the function. It was repeated three times in one function.
@@ -355,7 +368,10 @@ func (ms *moduleSocket) parseFileStream(ctx context.Context, packet *Packet) (bo | |||
} | |||
|
|||
tempDir := filepath.Join(os.TempDir(), "vx-store") | |||
os.Mkdir(tempDir, 0700) | |||
err := os.Mkdir(tempDir, 0700) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good example how linter message is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that you returned an error instead of wrapping it with if err != nil { log.. }
, and this considered as good fix. I mentioned that approach with logging is incorrect fix, in other comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK) I understand
will reopen later for new version of code |
Work with code for linter.