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

Refactor AddParam to AddParamIfExist #29834

Merged
merged 1 commit into from
Mar 16, 2024

Conversation

wxiaoguang
Copy link
Contributor

When read the code: pager.AddParam(ctx, "search", "search"), the question always comes: What is it doing? Where is the value from? Why "search" / "search" ?

Now it is clear: pager.AddParamIfExist("search", ctx.Data["search"])

@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 16, 2024
@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Mar 16, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 16, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 16, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 16, 2024
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 16, 2024

After this refactoring, we can see the a lot of old code could be simplified & clarified:

	paginator.AddParam("mode", "Mode")

TO

	paginator.AddParamString("mode", mode)

Because there are already clear variables "mode". As a refactoring PR, old code is left as-is. Leave more improvements to the future.

@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 Mar 16, 2024
@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 Mar 16, 2024
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.

Yep, this is better.

@KN4CK3R KN4CK3R merged commit e0ea381 into go-gitea:main Mar 16, 2024
26 checks passed
@wxiaoguang wxiaoguang deleted the refactor-addparam branch March 16, 2024 10:09
6543 pushed a commit that referenced this pull request Mar 16, 2024
Follow #29834

Remove AddParamIfExist, use "AddParamString" instead, it should clearly
know what is being added into the parameters.
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 16, 2024
* giteaofficial/main: (28 commits)
  Forbid jQuery `.prop` and fix related issues (go-gitea#29832)
  Fix wrong test for TestPullView_CodeOwner (go-gitea#29838)
  Forbid HTML injection using jQuery (go-gitea#29843)
  Meilisearch double quote on "match" query (go-gitea#29740)
  Forbid variables containing jQuery collections not having the `$` prefix (go-gitea#29839)
  Remove AddParamIfExist(AddParam) (go-gitea#29841)
  Refactor markdown attention render (go-gitea#29833)
  Refactor code_indexer to use an SearchOptions struct for PerformSearch (go-gitea#29724)
  Refactor AddParam to AddParamIfExist (go-gitea#29834)
  Forbid jQuery AJAX (go-gitea#29818)
  Remove jQuery AJAX from the notifications (go-gitea#29817)
  Light theme color enhancements (go-gitea#29830)
  Better highlighting of archved labels (go-gitea#29749)
  Remove the `time-since` class (go-gitea#29826)
  Remove jQuery AJAX from the project page (go-gitea#29814)
  Upgrade `htmx` to v1.9.11 (go-gitea#29821)
  Dark theme color enhancements (go-gitea#29822)
  Remove jQuery AJAX from the comment edit box (go-gitea#29812)
  Tweak labeler (go-gitea#29809)
  Fix `for` attribute not pointing to the ID of the color picker (go-gitea#29813)
  ...

# Conflicts:
#	routers/web/user/home.go
silverwind pushed a commit to silverwind/gitea that referenced this pull request Mar 17, 2024
When read the code: `pager.AddParam(ctx, "search", "search")`, the
question always comes: What is it doing? Where is the value from? Why
"search" / "search" ?

Now it is clear: `pager.AddParamIfExist("search", ctx.Data["search"])`
silverwind pushed a commit to silverwind/gitea that referenced this pull request Mar 17, 2024
Follow go-gitea#29834

Remove AddParamIfExist, use "AddParamString" instead, it should clearly
know what is being added into the parameters.
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 14, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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.

5 participants