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 MustGet function #2

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Add MustGet function #2

merged 1 commit into from
Jan 25, 2024

Conversation

AuroraTea
Copy link
Contributor

Reason

  1. Many cases will do IsSpecified() and isNull() judgment before calling Get().

  2. Compared to val, _ := foo.Get(), foo.MustGet() in many scenarios will be more atomic, the call logic does not have to truncate or be separated from other logic (such as assigning a value to another struct).

Test

=== RUN   ExampleNewNullNullable
--- PASS: ExampleNewNullNullable (0.00s)
=== RUN   ExampleNewNullableWithValue
--- PASS: ExampleNewNullableWithValue (0.00s)
=== RUN   ExampleNullable_marshalRequired
--- PASS: ExampleNullable_marshalRequired (0.00s)
=== RUN   ExampleNullable_marshalOptional
--- PASS: ExampleNullable_marshalOptional (0.00s)
=== RUN   ExampleNullable_unmarshalRequired
--- PASS: ExampleNullable_unmarshalRequired (0.00s)
=== RUN   ExampleNullable_unmarshalOptional
--- PASS: ExampleNullable_unmarshalOptional (0.00s)
PASS

=== RUN   TestNullable
--- PASS: TestNullable (0.00s)
PASS

@jamietanna jamietanna self-requested a review January 14, 2024 18:23
@jamietanna
Copy link
Member

Hey @AuroraTea thanks for raising this!

Would you mind sharing more details about how you'd want to use this? In the library's core use-case we were expecting it to be used in HTTP services, where a panic wouldn't be ideal, but if there are cases you'd be looking to use this that'd make sense, then sure.

I get the point that (hopefully) a consumer would be doing the IsSpecified and IsNull checks before they call Get

@sonasingh46
Copy link

This PR makes sense to me to have a MustGet kind of utility. I have too felt that, it is otherwise bit messy to use in the code.

if request.IsNull() {
		// do something
	} else if request.IsSpecified() {
		got, err := request.Get()
		if err != nil {
			return err
		}
		// do something
	}

Using MustGet, the above simplifies to :

if request.IsNull() {
		// do something
	} else if request.IsSpecified() {
		got := request.MustGet()
		// do something
	}

@AuroraTea
Copy link
Contributor Author

AuroraTea commented Jan 18, 2024

@sonasingh46 Thank you! Mostly.

@jamietanna Sorry I'm busy recently. Mostly like sonasingh46's Example.
Moreover:

	//IsNull and IsSpecified handling
	f1, _ := req.field1.Get()
	f2, _ := req.field2.Get()
	f3, _ := req.field3.Get()
	sql, args, _ := sqpg.
		Select("*").
		From("products").
		Where(sq.Eq{"id": f1}).
		Where(sq.Eq{"name": f2}).
		Where(sq.Eq{"supplier": f3}).
		ToSql()

vs

	//IsNull and IsSpecified handling
	sql, args, _ := sqpg.
		Select("*").
		From("products").
		Where(sq.Eq{"id": req.field1.MustGet()}).
		Where(sq.Eq{"name": req.field2.MustGet()}).
		Where(sq.Eq{"supplier": req.field3.MustGet()}).
		ToSql()

No additional variable (Especially additional variable names), No additional logic.
And it's why someone not only me don't like pointer for unMarshal (And of course can't distinguish null / notSpecified )

In the library's core use-case we were expecting it to be used in HTTP services, where a panic wouldn't be ideal

Just because it's used in HTTP services I'm going to assume that the user should have judged it before calling: the backend needs to validate, and those who don't need to check for IsNull and IsSpecified would not choose the nullable approach.

Also adding MustGet() and not breaking Get() greatly reduces the possibility of misuse. The reason for the panic is only because functions with the 'Must' prefix have such a non-mandatory convention. And the 'Must' prefix it's the most widely used... also for nullable's readable.

@jamietanna
Copy link
Member

@AuroraTea I can't seem to push back to your fork, would you mind un-protecting your main branch? Or re-raising from a new branch?

I've got the following changes to add:

diff --git nullable_example_test.go nullable_example_test.go
index bb0394c..c391817 100644
--- nullable_example_test.go
+++ nullable_example_test.go
@@ -252,6 +252,7 @@ func ExampleNullable_unmarshalRequired() {
 		return
 	}
 	fmt.Printf("obj.Name.Get(): %#v <nil>\n", val)
+	fmt.Printf("obj.Name.MustGet(): %#v\n", obj.Name.MustGet())
 	fmt.Println("---")
 
 	// when it's set explicitly to a specific value
@@ -274,6 +275,7 @@ func ExampleNullable_unmarshalRequired() {
 		return
 	}
 	fmt.Printf("obj.Name.Get(): %#v <nil>\n", val)
+	fmt.Printf("obj.Name.MustGet(): %#v\n", obj.Name.MustGet())
 	fmt.Println("---")
 
 	// Output:
@@ -289,11 +291,13 @@ func ExampleNullable_unmarshalRequired() {
 	// obj.Name.IsSpecified(): true
 	// obj.Name.IsNull(): false
 	// obj.Name.Get(): "" <nil>
+	// obj.Name.MustGet(): ""
 	// ---
 	// Value:
 	// obj.Name.IsSpecified(): true
 	// obj.Name.IsNull(): false
 	// obj.Name.Get(): "foo" <nil>
+	// obj.Name.MustGet(): "foo"
 	// ---
 }

And then a tweak on the commit message (which I'm happy to do at time of merge)

Add `MustGet` function

In the cases where a user has explicitly checked `IsSpecified()` and
`!IsNull()`, calling `Get` only to discard the `err` can be a bit
awkward, so instead we can introduce a `MustGet` method to retrieve the
value and panic if there is an error, as is common to do with Go
libraries.

@jamietanna
Copy link
Member

Happy to merge afterwards, appreciate you raising this 🚀

@AuroraTea
Copy link
Contributor Author

AuroraTea commented Jan 20, 2024

@jamietanna
I seem to have already added those test cases from the beginning? 😀

For commit message, I have now removed the protection rules. Give it a try, and if it doesn't work, I'll attempt re-raising from a new branch.

Surprisingly it's possible to change the commit message of a commit already submitted to GitHub. When you have time, could you please teach me how to do it?

@jamietanna
Copy link
Member

Hey @AuroraTea sorry it wasn't clear in my diff - I can see that ExampleNullable_unmarshalOptional has been updated to add the MustGet, but I can't see it in ExampleNullable_unmarshalRequired - would you mind adding it in there too?

I'm happy for you to amend.

Sure! The quickest way to do so is:

# assuming you're on your branch
# edit the files
git add -p
git commit --amend
# write your new commit message
# git push -f

That's it 🚀

Note that force pushing can be dangerous and destructive, so avoid doing it on branches where other folks may be contributing / using (i.e. main) - there are some ways to more safely force push (i.e.) and when solo working it's fine to do so, but a good practice to avoid shared/"public" branches like main being force pushed.

In the cases where a user has explicitly checked `IsSpecified()` and
`!IsNull()`, calling `Get` only to discard the `err` can be a bit
awkward, so instead we can introduce a `MustGet` method to retrieve the
value and panic if there is an error, as is common to do with Go
libraries.
@AuroraTea
Copy link
Contributor Author

push-f is fun😁

I used to have multiple commits but use squash or rebase for merging PR.

@jamietanna jamietanna changed the title feat: add function MustGet Add MustGet function Jan 25, 2024
@jamietanna jamietanna added the enhancement New feature or request label Jan 25, 2024
Copy link
Member

@jamietanna jamietanna left a comment

Choose a reason for hiding this comment

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

Thank you 🚀

@jamietanna jamietanna merged commit 5c8aab1 into oapi-codegen:main Jan 25, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants