Skip to content

Conversation

cristaloleg
Copy link
Contributor

Found by go-critic sloppyLen checker

Log:

modules/user/user_test.go:21:5 sloppyLen len(user) <= 0 can be len(user) == 0
models/issue_label.go:137:5 sloppyLen len(labelName) <= 0 can be len(labelName) == 0
models/org.go:680:5 sloppyLen len(repoIDs) <= 0 can be len(repoIDs) == 0
models/org.go:708:5 sloppyLen len(repoIDs) <= 0 can be len(repoIDs) == 0
models/ssh_key.go:379:5 sloppyLen len(key.Fingerprint) <= 0 can be len(key.Fingerprint) == 0
routers/user/home.go:216:5 sloppyLen len(userRepoIDs) <= 0 can be len(userRepoIDs) == 0
routers/api/v1/api.go:83:6 sloppyLen len(sudo) <= 0 can be len(sudo) == 0
modules/auth/auth.go:39:6 sloppyLen len(tokenSHA) <= 0 can be len(tokenSHA) == 0

@codecov-io
Copy link

codecov-io commented Oct 19, 2018

Codecov Report

Merging #5120 into master will decrease coverage by <.01%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5120      +/-   ##
=========================================
- Coverage    37.4%   37.4%   -0.01%     
=========================================
  Files         308     308              
  Lines       45612   45612              
=========================================
- Hits        17062   17060       -2     
- Misses      26093   26095       +2     
  Partials     2457    2457
Impacted Files Coverage Δ
routers/user/home.go 41.64% <0%> (ø) ⬆️
models/ssh_key.go 38.96% <0%> (ø) ⬆️
routers/api/v1/api.go 75.37% <100%> (ø) ⬆️
models/issue_label.go 59.51% <100%> (ø) ⬆️
modules/auth/auth.go 55.24% <100%> (ø) ⬆️
models/org.go 68.6% <50%> (ø) ⬆️
models/repo_list.go 62.2% <0%> (-1.17%) ⬇️

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 583b1b8...71e2a98. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 19, 2018
@lunny
Copy link
Member

lunny commented Oct 20, 2018

Is that really usable?

@cristaloleg
Copy link
Contributor Author

  1. We're checking 100% non-negative value for a negative value, so it's false all the time. It's redundant and brings a noise in the code.
  2. Also grepping code for == 0 (to quickly find checks for an empty string/slice/map) will not be found, we need explicitly grep <= 0, this brings unneeded complexity for new contributors 😉

@lafriks lafriks added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Oct 20, 2018
@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 20, 2018
@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 20, 2018
@techknowlogick techknowlogick merged commit 5a4648c into go-gitea:master Oct 20, 2018
@lunny lunny added this to the 1.7.0 milestone Oct 21, 2018
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants