Allow any type for range query parameters, preserve numeric strings #65
Conversation
| @@ -56,7 +56,7 @@ func (fb *Firebase) EndAt(value string) *Firebase { | |||
| func (fb *Firebase) OrderBy(value string) *Firebase { | |||
| c := fb.copy() | |||
| if value != "" { | |||
| c.params.Set(orderByParam, escapeString(value)) | |||
| c.params.Set(orderByParam, fmt.Sprintf(`%q`, strings.Trim(value, `"`))) | |||
There was a problem hiding this comment.
since the escapeParameter function already does this, can we call that here?
There was a problem hiding this comment.
I was wondering about this, but as orderBy must take a string parameter, I thought there's not much point in going back and forth with types. (Note that I haven't changed the parameter type either.)
But sure, we can call that too. It would still work :)
|
Thanks for making a PR! This particular logic was something that I had questioned for a while and I think your solution hits the nail right on the head! However, instead of changing the API, what do you think about adding another set of functions ( |
|
To be honest, I'm biased towards my version (with the original logic being fragile depending on the input). But my sneaky behavioral change is just as dangerous, as it doesn't cause any compilation errors, just potentially break things at runtime right away... Not an easy choice, so I'll leave it to you 馃槈 as the lib is yours. If you opt for the separate functions, I'll bring them back in the morning (and apply your comment above). |
|
So I took the weekend to think about this a bit and I think I would like to go with having separate functions. While I agree with you that the behavior is a bit funky - we would potentially break other clients by changing it. However... I've been wanting to create a v2 branch for a long time and this particular issue is motivation enough to get started. In the |
|
Alright, thanks for the update. I'll juggle back the previous implementations and add mine as separate functions later today. |
|
@vzsg Can you update the PR with the changes we spoke about? |
|
It's been almost two months already? Sure, I'll have a go at it this weekend. I'll start over, because reimplementing the old behavior is highly demotivating. |
|
Closing in favor of #74. |
This PR has a breaking change to how query parameters StartAt, EndAt and EqualTo work.
Previously these functions took a string as a parameter, tried to parse it into integer or boolean, and if none succeeded, escaped it with quotes.
This behavior is incorrect in my opinion, as:
"true" != trueand"10231232" != 10231232, leaving the user of the library baffled why nothing is being returned*.Instead, I modified the functions to take an empty interface instead, and use a type switch to decide if it should be quoted or just printed to string without quotes. The
TestEscapeParametertest shows all the expected conversions.