Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Refactor compare router param parse#36105

New issue

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

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
lunny wants to merge29 commits intogo-gitea:main
base:main
Choose a base branch
Loading
fromlunny:lunny/refactor_compare

Conversation

@lunny
Copy link
Member

@lunnylunny commentedDec 8, 2025•
edited
Loading

Bugs fix extracted to#36166

@lunnylunny added the type/refactoringExisting code has been cleaned up. There should be no new functionality. labelDec 8, 2025
@GiteaBotGiteaBot added the lgtm/need 2This PR needs two approvals by maintainers to be considered for merging. labelDec 8, 2025
@github-actionsgithub-actionsbot added modifies/apiThis PR adds API routes or modifies them modifies/goPull requests that update Go code labelsDec 8, 2025
Co-authored-by: techknowlogick <techknowlogick@gitea.io>Signed-off-by: Lunny Xiao <xiaolunwen@gmail.com>
@a1012112796
Copy link
Member

please have a look at#36116

lunny reacted with thumbs up emoji

@lunnylunny marked this pull request as ready for reviewDecember 10, 2025 23:25
@lunnylunny changed the titleRefactor compare router param parseRefactor compare router param parse and fix bugsDec 11, 2025
@lunny
Copy link
MemberAuthor

please have a look at#36116

This has been included and it's ready to review now.

@GiteaBotGiteaBot added lgtm/need 1This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2This PR needs two approvals by maintainers to be considered for merging. labelsDec 11, 2025
@lunnylunny added this to the1.26.0 milestoneDec 12, 2025
@lunnylunny added backport/doneAll backports for this PR have been created backport/v1.25 labelsDec 12, 2025
@GiteaBotGiteaBot added lgtm/need 1This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2This PR needs two approvals by maintainers to be considered for merging. labelsDec 13, 2025
Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Comment on lines +87 to +89
ifrouterParam=="" {
return&CompareRouterReq{},nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it valid argument?

What will&CompareRouterReq{} compare?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

It's valid. Then both base repository and head repository are the same. The base branch and head branch are the default branch of base repository. The behavior is the same as Github. i.e.https://github.com/go-gitea/gitea/compare


ci.BaseBranch=util.Iif(compareReq.BaseOriRef=="",baseRepo.DefaultBranch,compareReq.BaseOriRef)
ci.HeadBranch=util.Iif(compareReq.HeadOriRef=="",ci.HeadRepo.DefaultBranch,compareReq.HeadOriRef)
ci.DirectComparison=compareReq.DirectComparison()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just useci (parse the route parameter intoCompareInfo) ? What's the purpose of passingcompareReq intoci?

Copy link
MemberAuthor

@lunnylunnyDec 15, 2025•
edited
Loading

Choose a reason for hiding this comment

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

It's easier to write the unit test for the router param parsing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't see why it is "easier"

Now you have 3 different structs for the same purpose:

  • CompareRouterReq
  • parseCompareInfoResult
  • CompareInfo

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

TheGetCompareInfo will be used without a compare router parsing, so I think it's better to keep it as two functions. I extract bugs fix to another PR and will only keep refactor code here. I also sent04e7382 to merge the 3 compare info structures.

@lunnylunny removed backport/doneAll backports for this PR have been created backport/v1.25 type/bug labelsDec 15, 2025
@lunnylunny changed the titleRefactor compare router param parse and fix bugsRefactor compare router param parseDec 15, 2025
@lunnylunny marked this pull request as draftDecember 15, 2025 20:16
lunny added a commit that referenced this pull requestDec 17, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@wxiaoguangwxiaoguangwxiaoguang left review comments

@silverwindsilverwindsilverwind approved these changes

@techknowlogicktechknowlogickAwaiting requested review from techknowlogick

@a1012112796a1012112796Awaiting requested review from a1012112796

@kerwin612kerwin612Awaiting requested review from kerwin612

@Zettat123Zettat123Awaiting requested review from Zettat123

Assignees

No one assigned

Labels

lgtm/need 1This PR needs approval from one additional maintainer to be merged.modifies/apiThis PR adds API routes or modifies themmodifies/goPull requests that update Go codetype/refactoringExisting code has been cleaned up. There should be no new functionality.

Projects

None yet

Milestone

1.26.0

Development

Successfully merging this pull request may close these issues.

8 participants

@lunny@a1012112796@wxiaoguang@silverwind@techknowlogick@kerwin612@Zettat123@GiteaBot
[8]ページ先頭
©2009-2025 Movatter.jp