| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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 |
Sorry, something went wrong.
|
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
}
|
Sorry, something went wrong.
|
@sonasingh46 Thank you! Mostly. @jamietanna Sorry I'm busy recently. Mostly like sonasingh46's Example. //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.
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. |
Sorry, something went wrong.
|
@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) DetailsAdd `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. |
Sorry, something went wrong.
|
Happy to merge afterwards, appreciate you raising this 🚀 |
Sorry, something went wrong.
|
@jamietanna 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? |
Sorry, something went wrong.
|
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 -fThat'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. |
Sorry, something went wrong.
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.
|
push-f is fun😁 I used to have multiple commits but use squash or rebase for merging PR. |
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you 🚀
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Reason
Many cases will do IsSpecified() and isNull() judgment before calling Get().
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