Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor some functions to support ctx as first parameter #21878

Merged
merged 12 commits into from Dec 3, 2022
Merged

refactor some functions to support ctx as first parameter #21878

merged 12 commits into from Dec 3, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 20, 2022

No description provided.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Nov 20, 2022
KN4CK3R
KN4CK3R reviewed Nov 20, 2022
View reviewed changes
models/activities/action.go Outdated Show resolved Hide resolved
models/organization/team.go Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 20, 2022
@lunny lunny requested review from KN4CK3R, 6543 and delvh November 29, 2022 14:12
KN4CK3R
KN4CK3R reviewed Nov 29, 2022
View reviewed changes
models/repo/repo.go Outdated Show resolved Hide resolved
modules/convert/convert.go Outdated Show resolved Hide resolved
services/lfs/server.go Outdated Show resolved Hide resolved
delvh
delvh reviewed Nov 29, 2022
View reviewed changes
services/repository/transfer.go
@@ -44,7 +44,7 @@ func TransferOwnership(doer, newOwner *user_model.User, repo *repo_model.Reposit
}
repoWorkingPool.CheckOut(fmt.Sprint(repo.ID))

newRepo, err := repo_model.GetRepositoryByID(repo.ID)
newRepo, err := repo_model.GetRepositoryByID(db.DefaultContext, repo.ID)
Copy link
Member

Choose a reason for hiding this comment

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

That's a function where I think a custom context is really needed:

From what I've seen so far, we need this especially for functions that update or delete something, right?
Let it simply be to define a transaction.
And if TransferOwnership does not modify the database in a potentially destructive way, what does?
Also, there are currently at least 4 calls to the default context here…

Copy link
Member

Choose a reason for hiding this comment

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

Or what kinds of functions need the context especially?
Because you seem to have it added mainly for GET functions, and not for UPDATE/ DELETE functions.
So, is there a guideline what should get a ctx parameter, or is it simply whatever we think needs it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or what kinds of functions need the context especially? Because you seem to have it added mainly for GET functions, and not for UPDATE/ DELETE functions. So, is there a guideline what should get a ctx parameter, or is it simply whatever we think needs it?

This need a separate refactor PR, we need move some codes from models.TransferOwnership to service layer TransferOwnership.

models/org_team.go Show resolved Hide resolved
models/org_team.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2022 •
edited

Codecov Report

Merging #21878 (2885c0f) into main (8698458) will increase coverage by 48.14%.
The diff coverage is 69.48%.

@@            Coverage Diff            @@
##           main   #21878       +/-   ##
=========================================
+ Coverage      0   48.14%   +48.14%     
=========================================
  Files         0     1037     +1037     
  Lines         0   141393   +141393     
=========================================
+ Hits          0    68073    +68073     
- Misses        0    65180    +65180     
- Partials      0     8140     +8140
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
models/issues/reaction.go 79.91% <0.00%> (ø)
models/org.go 45.33% <0.00%> (ø)
models/perm/access/access.go 62.66% <0.00%> (ø)
modules/context/context.go 69.29% <0.00%> (ø)
modules/repository/generate.go 32.58% <0.00%> (ø)
modules/repository/init.go 52.10% <0.00%> (ø)
routers/api/v1/repo/issue_comment.go 59.05% <0.00%> (ø)
routers/api/v1/repo/key.go 20.17% <0.00%> (ø)
routers/private/hook_post_receive.go 63.53% <0.00%> (ø)
... and 1150 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lunny
Copy link
Member Author

lunny commented Nov 30, 2022

@KN4CK3R @delvh done.

@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 Nov 30, 2022
@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 Dec 2, 2022
@lafriks lafriks added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 2, 2022
@lunny lunny merged commit 0a7d3ff into go-gitea:main Dec 3, 2022
@lunny lunny deleted the lunny/more_refactor branch December 3, 2022 02:48
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 3, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 3, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
6e6b6e6 
* giteaofficial/main:
  docs: add `Edit this page` (go-gitea#21981)
  refactor some functions to support ctx as first parameter (go-gitea#21878)
  Remove deprecated packages & staticcheck fixes (go-gitea#22012)
  Add pnpm to packages/overview (go-gitea#22008)
  Update to Alpine 3.17 (go-gitea#21904)
  Update gitea-vet to check FSFE REUSE (go-gitea#22004)
  Multiple improvements for comment edit diff (go-gitea#21990)
  Remove session in api tests (go-gitea#21984)
  Remove duplicate "Actions" label in mobile view (go-gitea#21974)
  Fix generate index failure possibility on postgres (go-gitea#21998)
  Use path not filepath in template filenames (go-gitea#21993)
  Update chroma to v2.4.0 (go-gitea#22000)
  Util type to parse ref name (go-gitea#21969)
  Skip initing LFS storage if disabled (go-gitea#21996)
  Fix parallel creating commit status bug with tests (go-gitea#21911)
  Skip initing disabled storages (go-gitea#21985)
  Fix leaving organization bug on user settings -> orgs (go-gitea#21983)
  Fix typos (go-gitea#21979)
  Normalize `AppURL` according to RFC 3986 (go-gitea#21950)
@AbdulrhmnGhanem AbdulrhmnGhanem mentioned this pull request Dec 30, 2022
Merged
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KN4CK3R KN4CK3R KN4CK3R left review comments

@lafriks lafriks lafriks approved these changes

@delvh delvh delvh approved these changes

@6543 6543 Awaiting requested review from 6543

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lunny @codecov-commenter @lafriks @KN4CK3R @delvh @GiteaBot