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

Introduce generic type values.NullableValue and reimplement OptionalBool #29341

Closed
wants to merge 2 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 23, 2024

Purpose

Since all Go basic types are not nullable, and nil has no type for default, but sometimes we need a nullable type. This PR introduces a new package modules/values and a generic type NullableValue which can resolve the defect of Golang types.

What's changed

And this PR also reimplemented util.OptionalBool with the introduced modules/values, the new implementation will only affect the util.go file and less places. Most places reference util.OptionalBool can be kept and works as before.
P.S. A potential bug was found because at the previous implementation util.OptionalBoolNone == 0, so you can always use 0 as that value. But in the new implementation, they are different types.

Next step

If this PR merged, many other places which needs a nullable type could reuse this infrastructure.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 23, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 23, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 23, 2024
@KN4CK3R
Copy link
Member

KN4CK3R commented Feb 23, 2024

We should decide for one: optional.Option or NullableValue.
Some discussion on the naming of the Option type: #28733 (comment)

@wxiaoguang
Copy link
Contributor

I do not think it needs to introduce a new type, IMO the current optional just works.

@lunny
Copy link
Member Author

lunny commented Feb 23, 2024

OK. I just confused by the function name Some. I will close this one and we can have more discussion about that function name. https://github.com/go-gitea/gitea/pull/29329/files#r1500305151

@lunny lunny closed this Feb 23, 2024
@lunny lunny deleted the lunny/values branch February 23, 2024 08:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 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. size/M Denotes a PR that changes 30-99 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.

None yet

4 participants