Skip to content

crud: refactor optional types to use go-option #498

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.

Sign up for GitHub

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
Open
babyTsakhes wants to merge 2 commits into
base: master
Choose a base branch
from
Open

crud: refactor optional types to use go-option #498

babyTsakhes wants to merge 2 commits into from

Conversation

@babyTsakhes
Copy link
Collaborator

@babyTsakhes babyTsakhes commented Nov 18, 2025

  • Remove: OptUint, OptInt, OptFloat64, OptString, OptBool, OptTuple.
  • Remove: MakeOptUint, MakeOptInt, MakeOptFloat64, MakeOptString, MakeOptBool, MakeOptTuple.
  • Add: OptAny = option.Generic[interface{}] .
  • Add: MakeOptAny constructor.
  • Update: All option structs to use option.* types.
  • Fix: Test type inconsistencies.

Changed #492

@babyTsakhes
Copy link
Collaborator Author

babyTsakhes commented Nov 18, 2025

Later, I'll add the Tuple and Any type to the go-option library and make the code even more typesafe.

oleg-jukovec
oleg-jukovec requested changes Nov 25, 2025
View reviewed changes
@babyTsakhes babyTsakhes marked this pull request as ready for review November 25, 2025 16:26
@babyTsakhes babyTsakhes force-pushed the babyTsakhes/gh-492-replace-usage-of-optional-types-in-crud-with-go-option branch 2 times, most recently from 499ecc6 to 21bd65d Compare December 1, 2025 18:36
@babyTsakhes
Copy link
Collaborator Author

babyTsakhes commented Dec 1, 2025

The required changes have been added.

@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Dec 1, 2025

The commit message should be something like:

iproto: add missing IPROTO feature flags to greeting negotiation

The welcome flags for the IPROTO_FEATURE_IS_SYNC functions have been added and
IPROTO_FEATURE_INSERT_ARROW for IPROTO_FEATURE_INSERT_ARROW protocol functions.
Changed it CHANGELOG.md in the previous version, it was in the wrong version.

Changed #466

->

doc: move entry about IPROTO to the right place

Follows #466

oleg-jukovec
oleg-jukovec requested changes Dec 1, 2025
View reviewed changes
@babyTsakhes babyTsakhes force-pushed the babyTsakhes/gh-492-replace-usage-of-optional-types-in-crud-with-go-option branch 3 times, most recently from f2c9c14 to 6ac2dcf Compare December 1, 2025 22:52
@oleg-jukovec
Copy link
Collaborator

oleg-jukovec commented Dec 4, 2025

Please, rebase on the master branch.

@babyTsakhes babyTsakhes force-pushed the babyTsakhes/gh-492-replace-usage-of-optional-types-in-crud-with-go-option branch from 6ac2dcf to 161f48c Compare December 4, 2025 21:27
iproto: add missing IPROTO feature flags to greeting negotiation
ab05008 
The welcome flags for the IPROTO_FEATURE_IS_SYNC functions have been added and
IPROTO_FEATURE_INSERT_ARROW for IPROTO_FEATURE_INSERT_ARROW protocol functions.
Changed it CHANGELOG.md in the previous version, it was in the wrong version.

Changed #466
@babyTsakhes babyTsakhes force-pushed the babyTsakhes/gh-492-replace-usage-of-optional-types-in-crud-with-go-option branch from 161f48c to 154d996 Compare December 4, 2025 21:31
oleg-jukovec
oleg-jukovec requested changes Dec 5, 2025
View reviewed changes
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

The first commit is useless at now.

(You've move the changelog entry in the first commit and move back it in the second one).

oleg-jukovec
oleg-jukovec requested changes Dec 5, 2025
View reviewed changes
@babyTsakhes babyTsakhes force-pushed the babyTsakhes/gh-492-replace-usage-of-optional-types-in-crud-with-go-option branch from 154d996 to 7561615 Compare December 5, 2025 11:55
bigbes
bigbes reviewed Dec 5, 2025
View reviewed changes
crud/insert.go
import (
"context"

"github.com/vmihailenco/msgpack/v5"
Copy link
Collaborator

@bigbes bigbes Dec 5, 2025

Choose a reason for hiding this comment

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

pls remove this unnecessary change.

Copy link
Collaborator Author

@babyTsakhes babyTsakhes Dec 8, 2025

Choose a reason for hiding this comment

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

Thank you! Changes are ready.

bigbes
bigbes reviewed Dec 5, 2025
View reviewed changes
@babyTsakhes
Copy link
Collaborator Author

babyTsakhes commented Dec 5, 2025

In general, I rebased to the master, after that the TestBox_Sugar_Schema_UserGrant_NoSu test crashed. I'm sorting it out now, it's not ready yet.

@babyTsakhes babyTsakhes force-pushed the babyTsakhes/gh-492-replace-usage-of-optional-types-in-crud-with-go-option branch from 7561615 to b941191 Compare December 5, 2025 14:04
@babyTsakhes
Copy link
Collaborator Author

babyTsakhes commented Dec 5, 2025

Maybe it's the test itself. Correct me, but it only crashes in 4 tests. Everything works locally.

@babyTsakhes
Copy link
Collaborator Author

babyTsakhes commented Dec 5, 2025

So the changes are ready, please take a look.

oleg-jukovec
oleg-jukovec requested changes Dec 7, 2025
View reviewed changes
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

A one mirror problem still here.

@babyTsakhes babyTsakhes force-pushed the babyTsakhes/gh-492-replace-usage-of-optional-types-in-crud-with-go-option branch 2 times, most recently from 4062f0e to df8f5b8 Compare December 8, 2025 12:19
@babyTsakhes babyTsakhes force-pushed the babyTsakhes/gh-492-replace-usage-of-optional-types-in-crud-with-go-option branch from df8f5b8 to 885086a Compare December 8, 2025 13:00
crud: refactor optional types to use go-option
7fd3fed 
- Remove: OptUint, OptInt, OptFloat64, OptString, OptBool, OptTuple.
- Remove: MakeOptUint, MakeOptInt, MakeOptFloat64, MakeOptString, MakeOptBool, MakeOptTuple.
- Update: All option structs to use option.* types.
- Add type Any from go-option.
- Fix: Test type inconsistencies.

Closes #492
@babyTsakhes babyTsakhes force-pushed the babyTsakhes/gh-492-replace-usage-of-optional-types-in-crud-with-go-option branch from 885086a to 7fd3fed Compare December 8, 2025 13:07
@babyTsakhes
Copy link
Collaborator Author

babyTsakhes commented Dec 8, 2025

So the changes are ready, please take a look. Everything seems to be correct. But the TestSchemaUser_Revoke_WithoutSu test also dropped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bigbes bigbes bigbes left review comments

@oleg-jukovec oleg-jukovec oleg-jukovec approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@babyTsakhes @oleg-jukovec @bigbes