Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Less naked returns #25713

Merged
merged 19 commits into from
Jul 7, 2023
Merged

Less naked returns #25713

merged 19 commits into from
Jul 7, 2023

Conversation

6543
Copy link
Member

@6543 6543 commented Jul 5, 2023

just a step towards #25655

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 5, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 5, 2023
@6543 6543 added type/refactoring Existing code has been cleaned up. There should be no new functionality. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 5, 2023
@6543 6543 mentioned this pull request Jul 5, 2023
Merged
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 5, 2023
silverwind
silverwind approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I would have been rather lost if I had attempted some of these fixes, so the help is very welcome.

@GiteaBot GiteaBot 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 Jul 6, 2023
@6543 6543 requested a review from a team July 6, 2023 18:57
KN4CK3R
KN4CK3R reviewed Jul 6, 2023
View reviewed changes
Copy link
Member

@KN4CK3R KN4CK3R left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but some of the changed methods still have return names.

@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 6, 2023
@6543 6543 requested a review from KN4CK3R July 6, 2023 20:33
wxiaoguang
wxiaoguang requested changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have objection for removing the named return value.

For maintainability , IMO it's better to:

Keep the named return value (func foo() (canRead, canWrite bool)) and use full return statement (return canRead, canWrite or return false, false)

@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 7, 2023
@6543
Copy link
Member Author

6543 commented Jul 7, 2023

well the rest is just bike-shading in my opinion

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 7, 2023
@6543 6543 requested a review from wxiaoguang July 7, 2023 04:06
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Jul 7, 2023
wxiaoguang
wxiaoguang reviewed Jul 7, 2023
View reviewed changes
@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 7, 2023
wxiaoguang
wxiaoguang reviewed Jul 7, 2023
View reviewed changes
@wxiaoguang wxiaoguang self-requested a review July 7, 2023 04:17
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Jul 7, 2023
@wxiaoguang
Copy link
Contributor

Overall LGTM (2 nits above)

6543
6543 commented Jul 7, 2023
View reviewed changes
6543 and others added 2 commits July 7, 2023 06:45
@6543
Update modules/util/util.go
c8d3e58
@6543 @wxiaoguang
Update routers/web/web.go
4e2901f 
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
wxiaoguang
wxiaoguang approved these changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I meant ToRef itself but not only the comment #25713 (comment)

(only a nit, so LGTM)

@GiteaBot GiteaBot 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 Jul 7, 2023
@6543 6543 enabled auto-merge (squash) July 7, 2023 05:02
@6543 6543 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 7, 2023
@6543 6543 merged commit 8995046 into go-gitea:main Jul 7, 2023
@GiteaBot GiteaBot added this to the 1.21.0 milestone Jul 7, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 7, 2023
@6543 6543 deleted the less_naked_returns branch July 7, 2023 05:33
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 5, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@delvh delvh delvh left review comments

@silverwind silverwind silverwind approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@KN4CK3R KN4CK3R Awaiting requested review from KN4CK3R

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
6 participants
@6543 @wxiaoguang @silverwind @KN4CK3R @delvh @GiteaBot