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

Add optional.ExtractValue() to be able to handle Option type within interface{} value #29794

Closed

Conversation

6543
Copy link
Member

@6543 6543 commented Mar 14, 2024

as title ... add an helper func for optional.Option type(s)

because simple type cast wont work if you not know the exact T type too ... (or want your func handle all Option Types ...)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 14, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 14, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 14, 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.

What's the usecase of this?

modules/optional/option.go Outdated Show resolved Hide resolved
@6543
Copy link
Member Author

6543 commented Mar 14, 2024

usecase would be to make

pager.AddParam(ctx, "state", "State")

optional.Option aware etc...

@6543
Copy link
Member Author

6543 commented Mar 14, 2024

should I add this usecase in here too?

modules/optional/option.go Outdated Show resolved Hide resolved
modules/optional/option_test.go Outdated Show resolved Hide resolved
modules/optional/option_test.go Outdated Show resolved Hide resolved
modules/optional/option_test.go Outdated Show resolved Hide resolved
modules/optional/option_test.go Outdated Show resolved Hide resolved
modules/optional/option_test.go Outdated Show resolved Hide resolved
@6543 6543 changed the title Add optional.ExcractValue() to be able to handle Option type within interface{} value Add optional.ExtractValue() to be able to handle Option type within interface{} value Mar 14, 2024
@6543
Copy link
Member Author

6543 commented Mar 15, 2024

@lunny @KN4CK3R added an example usage

@KN4CK3R
Copy link
Member

KN4CK3R commented Mar 16, 2024

How/Where gets the optional.Option into ctx.Data?

@6543
Copy link
Member Author

6543 commented Mar 16, 2024

if we start to adopt and use optionals more widly instead of type refs ...
e.g.
https://github.com/go-gitea/gitea/pull/29726/files#diff-aa8a6ecda61a1825f948895bec48df575bee830dd5c3d25fd81977452041945cR131-R133

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 16, 2024

Actually IMO the design of AddParam is totally wrong. It should add the key/value explicitly, but not depending on the "ctx.Data"

If the AddParam could be implemented clearly, then no need the ExtractValue trick.


For example, when I read the code: pager.AddParam(ctx, "search", "search"), the question always comes into my mind: what is it doing? Where is the value from? Why "search" / "search" ?

@wxiaoguang
Copy link
Contributor

-> Refactor AddParam to AddParamIfExist #29834

@6543
Copy link
Member Author

6543 commented Mar 16, 2024

In this regards it looks like it is not jet needed, so I'll close this pull and everybody is free to pick that patch if needed in the future

@6543 6543 closed this Mar 16, 2024
@6543 6543 deleted the optional.Option-add-ExcractValue branch March 16, 2024 16:50
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants