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

refactor: decouple context from migration structs #33399

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TheFox0x7
Copy link
Contributor


I've started working on pulling out contexts from structs per golang conventions and I took migrations on first.
It should be fine apart from gogs which I have no idea how to decouple - safe for updating/forking gogs client.

Ideas and initial feedback are very much welcome.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 25, 2025
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 25, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jan 25, 2025
@wxiaoguang
Copy link
Contributor

I've started working on pulling out contexts from structs per golang conventions

I think it depends on whether it really brings enough benefits. Golang itself (official packages, for example: http) also store the ctx into struct field in many cases .........

@TheFox0x7
Copy link
Contributor Author

According to the go's blog post the only reason why net/http has context in its struct is backwards compatibility:
https://go.dev/blog/context-and-structs
And it's not recommended to store it there.

@wxiaoguang
Copy link
Contributor

According to the go's blog post the only reason why net/http has context in its struct is backwards compatibility: https://go.dev/blog/context-and-structs And it's not recommended to store it there.

Yup, that's same to Gitea's code base. New code seldom stores context into a struct.

Some old code already stores the context into the struct, which is not good, while not too bad if it hasn't caused any problem (for example: markup render also does so). That's what I meant "I think it depends on whether the refactoring really brings enough benefits."


For "gogs downloader", the NewClient is quite lightweight, if you'd like to make the downloader support context, maybe it could re-create the client in every function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/go Pull requests that update Go code size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants