Enable more linters #252

hanzei · 2020-04-25T18:25:50Z

Summary

This PR sync the golangci-lint config with the one from other repos. Hence, a lot of new linter got enabled. The config file is synced with mattermost/mattermost-plugin-starter-template#90.

Given the high number of changed lines, I tried to break the changes into multiple commits.

While most changes are just stylistic, I did comment on the more tricky one.

Ticket Link

Follow up on mattermost-community/mattermost-plugin-autolink#108

codecov-io · 2020-04-27T06:06:57Z

Codecov Report

Merging #252 into master will decrease coverage by 0.27%.
The diff coverage is 7.83%.

@@            Coverage Diff             @@
##           master     #252      +/-   ##
==========================================
- Coverage   19.69%   19.42%   -0.28%     
==========================================
  Files          10       10              
  Lines        2178     2193      +15     
==========================================
- Hits          429      426       -3     
- Misses       1725     1742      +17     
- Partials       24       25       +1

Impacted Files	Coverage Δ
server/configuration.go	`32.25% <0.00%> (ø)`
server/permalinks.go	`77.14% <ø> (ø)`
server/plugin.go	`4.30% <0.00%> (-0.36%)`	⬇️
server/webhook.go	`0.00% <0.00%> (ø)`
server/command.go	`10.11% <5.26%> (+0.05%)`	⬆️
server/subscriptions.go	`10.97% <9.09%> (+0.13%)`	⬆️
server/api.go	`6.88% <11.66%> (+0.15%)`	⬆️
server/utils.go	`61.32% <50.00%> (-0.64%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fcc0ad...2b10008. Read the comment docs.

hanzei · 2020-04-27T15:06:11Z

.golangci.yml

+    - path: server/webhook.go
+      linters:
+        - goconst


There are so many action types like "opened" in server/webhook.go that refactoring them into const's did make the code more readable.

hanzei · 2020-04-27T15:07:08Z

go.mod

 require (
-	github.com/Masterminds/sprig/v3 v3.0.2
-	github.com/google/go-github/v25 v25.1.1
+	github.com/Masterminds/sprig/v3 v3.1.0
+	github.com/google/go-github/v31 v31.0.0
 	github.com/gorilla/mux v1.7.4
-	github.com/mattermost/mattermost-server/v5 v5.18.0
-	github.com/pkg/errors v0.8.1
-	github.com/stretchr/testify v1.4.0
+	github.com/mattermost/mattermost-server/v5 v5.22.0
+	github.com/mholt/archiver/v3 v3.3.0
+	github.com/pkg/errors v0.9.1
+	github.com/stretchr/testify v1.5.1


I did also include an dependency update into this PR to help keep the number of PRs QA has to review low.

hanzei · 2020-04-27T15:09:19Z

server/plugin.go

-func (p *Plugin) CreateBotDMPost(userID, message, postType string) *model.AppError {
+// CreateBotDMPost posts a direct message using the bot account.
+// Any error are not returned and instead logged.
+func (p *Plugin) CreateBotDMPost(userID, message, postType string) {


Omitting the returned error and logging it instead just make the caller code simplere.

hanzei · 2020-04-27T15:10:41Z

server/template_test.go

@@ -360,7 +359,7 @@ func TestPushedCommitsTemplate(t *testing.T) {
 			Repo:   &pushEventRepository,
 			Sender: &user,
 			Forced: bToP(true),
-			Commits: []github.PushEventCommit{
+			Commits: []*github.HeadCommit{


github.PushEventCommit was removed from github.com/google/go-github. It's just a naming change.

server/webhook.go

hanzei · 2020-04-27T15:12:25Z

@bakurits Given that you touched a lot of the code base lately, a review from you would be appreciated.

server/api.go

server/plugin.go

server/subscriptions.go

server/webhook.go

agnivade

Looks great. Just some minor comments.

server/plugin.go

server/subscriptions.go

server/webhook.go

build/deploy/main.go

build/manifest/main.go

larkox

LGTM

server/api_test.go

server/command.go

hanzei · 2020-05-11T10:55:20Z

Waiting to merge some community PRs to not cause to many merge conflicts.

codecov-commenter · 2020-05-19T06:29:18Z

Codecov Report

Merging #252 into master will decrease coverage by 0.36%.
The diff coverage is 7.63%.

@@            Coverage Diff             @@
##           master     #252      +/-   ##
==========================================
- Coverage   18.50%   18.13%   -0.37%     
==========================================
  Files          10       10              
  Lines        2340     2371      +31     
==========================================
- Hits          433      430       -3     
- Misses       1883     1916      +33     
- Partials       24       25       +1

Impacted Files	Coverage Δ
server/configuration.go	`32.25% <0.00%> (ø)`
server/plugin.go	`4.27% <0.00%> (-0.18%)`	⬇️
server/webhook.go	`0.00% <0.00%> (ø)`
server/command.go	`10.11% <5.26%> (+0.05%)`	⬆️
server/subscriptions.go	`10.97% <9.09%> (+0.13%)`	⬆️
server/api.go	`5.72% <10.16%> (-0.13%)`	⬇️
server/utils.go	`61.32% <50.00%> (-0.64%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b124fd0...592a5e6. Read the comment docs.

hanzei · 2020-05-19T18:38:33Z

@DHaussermann This PR is ready for review. Given a lot of files got touched, please do a general test. Especially take a look at the interaction between the webapp and server, e.g. creating issue, RHS, LHS, etc.

hanzei · 2020-05-20T10:12:31Z

@DHaussermann The testing for this PR and the testing for the release are basically the same: Test everything. With that in mind, I'm wondering if you would be fine merging the PR as it is. By merging early double testing gets avoided.

DHaussermann

Tested and passed

No issues with build or deploy
Briefly smoke tested focusing on LHS, RHS and post menu options
No issues found

LGTM!
No changes need to release testing.

hanzei added 7 commits April 25, 2020 20:23

Update build system and config file

568c837

Auto fix

562fe9d

Fix simple errors

0b44be3

Enable goconst

135bb89

Enable errcheck

fd36a1c

Update dependencies

6b3bcf1

Fix typo

8be86b0

hanzei added the Work In Progress Not yet ready for review label Apr 25, 2020

hanzei added this to the v1.0.0 milestone Apr 25, 2020

hanzei changed the title ~~Enable for linters~~ Enable more linters Apr 27, 2020

Consistent import odering

c24908b

hanzei commented Apr 27, 2020

View reviewed changes

hanzei requested a review from larkox April 27, 2020 15:12

hanzei added 2: Dev Review Requires review by a core committer and removed Work In Progress Not yet ready for review labels Apr 27, 2020

hanzei requested a review from agnivade April 27, 2020 15:12

bakurits reviewed Apr 27, 2020

View reviewed changes

agnivade reviewed Apr 28, 2020

View reviewed changes

server/plugin.go Show resolved Hide resolved

server/subscriptions.go Outdated Show resolved Hide resolved

server/webhook.go Outdated Show resolved Hide resolved

build/deploy/main.go Show resolved Hide resolved

build/manifest/main.go Show resolved Hide resolved

Drop hasBeenNotified

203413d

hanzei requested a review from agnivade May 2, 2020 11:21

hanzei mentioned this pull request May 2, 2020

Properly wrap errors #254

Closed

agnivade approved these changes May 2, 2020

View reviewed changes

Add comment why crypto/sha1 is needed

0953348

hanzei mentioned this pull request May 6, 2020

[GH-255] Add tooltip for pull request and issue links #258

Merged

s/GET/http.MethodGet/g

fb01beb

larkox approved these changes May 8, 2020

View reviewed changes

server/api_test.go Show resolved Hide resolved

server/command.go Show resolved Hide resolved

Move all features to consts

27ac79a

hanzei added Work In Progress Not yet ready for review and removed 2: Dev Review Requires review by a core committer labels May 11, 2020

hanzei added 3: QA Review Requires review by a QA tester Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed Work In Progress Not yet ready for review labels May 11, 2020

hanzei added 4 commits May 16, 2020 08:12

Merge remote-tracking branch 'origin/master' into golangci-lint

3e086f1

Fix eslint warnings

2b10008

Merge remote-tracking branch 'origin/master' into golangci-lint

e4e2c0c

Merge remote-tracking branch 'origin/master' into golangci-lint

e991778

hanzei removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label May 19, 2020

jfrerich mentioned this pull request May 19, 2020

Bump version to v1.0.0 #274

Merged

hanzei requested a review from DHaussermann May 19, 2020 18:35

hanzei added 2 commits May 19, 2020 20:40

Update dependencies

51d0b50

go mod tidy

91ea349

Fix RHS title

592a5e6

hanzei mentioned this pull request May 21, 2020

/api/v1/connected should be publicly accessible #279

Merged

DHaussermann approved these changes May 21, 2020

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels May 21, 2020

hanzei merged commit 553bdea into master May 22, 2020

hanzei deleted the golangci-lint branch May 22, 2020 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable more linters #252

Enable more linters #252

hanzei commented Apr 25, 2020 •

edited

Loading

codecov-io commented Apr 27, 2020 •

edited

Loading

hanzei Apr 27, 2020

hanzei Apr 27, 2020

hanzei Apr 27, 2020

hanzei Apr 27, 2020

hanzei commented Apr 27, 2020

agnivade left a comment

larkox left a comment

hanzei commented May 11, 2020

codecov-commenter commented May 19, 2020 •

edited

Loading

hanzei commented May 19, 2020

hanzei commented May 20, 2020

DHaussermann left a comment •

edited by hanzei

Loading

Enable more linters #252

Enable more linters #252

Conversation

hanzei commented Apr 25, 2020 • edited Loading

Summary

Ticket Link

codecov-io commented Apr 27, 2020 • edited Loading

Codecov Report

hanzei Apr 27, 2020

Choose a reason for hiding this comment

hanzei Apr 27, 2020

Choose a reason for hiding this comment

hanzei Apr 27, 2020

Choose a reason for hiding this comment

hanzei Apr 27, 2020

Choose a reason for hiding this comment

hanzei commented Apr 27, 2020

agnivade left a comment

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

hanzei commented May 11, 2020

codecov-commenter commented May 19, 2020 • edited Loading

Codecov Report

hanzei commented May 19, 2020

hanzei commented May 20, 2020

DHaussermann left a comment • edited by hanzei Loading

Choose a reason for hiding this comment

hanzei commented Apr 25, 2020 •

edited

Loading

codecov-io commented Apr 27, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading

DHaussermann left a comment •

edited by hanzei

Loading