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

Fix option to handle type conversion #49

Merged
merged 4 commits into from
Jul 16, 2024
Merged

Conversation

mtsoltan
Copy link
Contributor

Conversion doesn't happen automatically when using mo.Option to accept from drivers, or inside gorm model structs.

The test cases assert the desired behavior, and fail on upstream before this PR, but pass after the changes made.

This supersedes #34, which can now be closed once this is successfully merged.

@mtsoltan
Copy link
Contributor Author

mtsoltan commented Jul 14, 2024

This also resolves #24

Edits by maintainers are allowed, in case you want to change something with my comment style. ^.^ Cheers!

@samber
Copy link
Owner

samber commented Jul 16, 2024

Thanks @mtsoltan for your first contribution.

Love it!

Could you please make it compatible with go < 1.22?

You can split it into 3 files:

  • option.go
  • option_go118.go
  • option_go122.go

A build condition can be added at the top of the file using //go:build !go1.22.

@mtsoltan
Copy link
Contributor Author

Check the last commit. I went through great deals of verification to make sure all access patterns of convertAssign inside the 1.18 standard library do not expose it. There's simply no way to access that function, so the only solution was to copy it :(

This introduces the debt of maintaining it with this PR, but honestly, given how stable that function is in the standard library of 1.18, I think it's a fair trade in order to achieve a working Scan.

Let me know what you think.

@@ -0,0 +1,22 @@
//go:build go1.22
// +build go1.22
Copy link
Owner

Choose a reason for hiding this comment

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

Did not know this syntax before.

It seems that it has been deprecated in go 1.17. So it would be unnecessary in this project (mo supports >= 1.18).

@samber samber merged commit 6745eff into samber:master Jul 16, 2024
7 checks passed
@samber
Copy link
Owner

samber commented Jul 16, 2024

-> v1.13.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants