CI: Add linter job to CI workflow #166
Conversation
|
@TheodosiouTh it seems there are some more linting issues once I ran it locally. Note that the github action you've just added doesn't run on this PR's CI so we have to make sure the check passes locally. Pasting the output: |
|
@preslavmihaylov Thank you for your input. |
TheodosiouTh
changed the title
|
Also, could you tweak the linter config a bit and include the Additionally, once you make that change, you can remove the gofmt step from the CI pipeline as its a subset of the step you're adding. |
|
Actually, we should use |
|
@preslavmihaylov I added |
|
@preslavmihaylov I have checked the code again and I do not get why the |
TheodosiouTh
changed the title
|
@preslavmihaylov I think the PR is ready for review, feel free to check it and give me feedback. |
main.go
Outdated
| printTodoErrs(todoErrs, *format) | ||
| err = printTodoErrs(todoErrs, *format) | ||
| if err != nil { | ||
| logger.Info("Error when printing todo errors" + err.Error()) |
There was a problem hiding this comment.
This should be a panic as we should never get here
There was a problem hiding this comment.
I changed the code to panic if this point is reached during execution.
testing/scenariobuilder/builder.go
Outdated
| w.Write(issuetracker.BuildResponseFor(s.issueTracker, issue, s.issues[issue])) | ||
| _, err := w.Write(issuetracker.BuildResponseFor(s.issueTracker, issue, s.issues[issue])) | ||
| if err != nil { | ||
| logger.Info("Error " + err.Error() + " occured, when writing issuetracker response of issue:" + issue) |
There was a problem hiding this comment.
This should be a panic as it should never happen
There was a problem hiding this comment.
I changed the code to panic if this point is reached during execution.
traverser/todoerrs/todoerrs.go
Outdated
| todoErrCallback(todoErr) | ||
| err = todoErrCallback(todoErr) | ||
| if err != nil { | ||
| logger.Info("couldn't run todo error callback: " + err.Error()) |
There was a problem hiding this comment.
We should instead return fmt.Errorf("received error from todo err callback: %w", err)
There was a problem hiding this comment.
I updated the code according to your suggestion.
traverser/comments/comments.go
Outdated
| prev = curr | ||
| } | ||
|
|
||
| curr = next | ||
| next = 0 | ||
| t.handleStateChange(filename, line, linecnt, prev, curr, next) | ||
| err := t.handleStateChange(filename, line, linecnt, prev, curr, next) | ||
| if err != nil { |
There was a problem hiding this comment.
These errors shouldn't be handled here as they are handled via the last return t.callbackErr.
Does the linter allow ignoring the error via _ = t.handleStateChange(...)?
Alternatively, you can change the signature of the function to not return an error and update it accordingly
There was a problem hiding this comment.
The linter does allow _ = t.handleStageChange(...) so I updated the code.
|
@preslavmihaylov I updated the code according to your suggestions. Also, I found a logic bug in Feel free to check the rest of the changes. |
Closes #165