-
Notifications
You must be signed in to change notification settings - Fork 493
Conversation
} | ||
|
||
JsonObject& Firebase::json() { | ||
_json = StaticJsonBuffer<200>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not reinitializing the _json
object is against "best practices` of ArduinoJson, see https://github.com/bblanchon/ArduinoJson/wiki/Avoiding-pitfalls#4-dont-reuse-the-same-jsonbuffer and https://github.com/bblanchon/ArduinoJson/wiki/FAQ#how-to-reuse-a-jsonbuffer
One way to do would be to push back decoding json
back to the sketch and only expose a way to get the raw body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also json()
is miss-leading because it's not clear that it will work if the user call it two times in a row (since parseObject
does mutate the string buffer to add \0
).
Current stance of Arduino library support is advertised here: |
String _data; | ||
public: | ||
FirebaseError error; | ||
JsonObject& json; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to expose this? It seems like we are exposing internal state. It is a mutable object that the client can change accidentally, when we don't actually need mutability. In addition the client needs to understand the FirebaseObject and the JasonObject. The latter is especially extra work as it involves hunting down the source for another library.
What if we keep this internal and expose the get, is, and operator[] in a immutable way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I agree. I took a shortcut because I didn't want to duplicate the API of JsonObject
up front (not only operator[]
but also every cast operator for literal value).
Also nesting the JsonObject
give us a way to have a result object that's either an error or some json, if we flatten it in the FirebaseObject
: we could document that you need to call error()
before accessing / casting anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
But still need to also forward the cast operators for string and number literal (and get/is).
It isn't critical at all but it still seems like it would be nice to have the option of whether you want to read a response or not. This does complicate things a bit, I was playing around with it. It gets complicated when you are reusing a connection because you need a bit of logic to maintain the state of the connection and ensure it is cleared before any subsequent calls. I think one advantage of creating a opaque class that we own as a return value is that we can have the freedom to evolve as necessary down the road without affecting existing users. Like if you are always asking the return value to process the result (i.e. calling ro.get(x) or ro[x] instead of ro.json[x]) than the return value could operate as is for now where it buffers the whole response be we can easily update it in the future to only read on demand if we want without requiring call-site changes. The other advantage, as stated in the line comment, is that we can allow the user to read one header file and know completely how to interact with us. Without having to hunt around in different libraries to find the answers. I think this should be a goal of ours, it makes a large difference from the user perspective. Atleast in my experience. |
Yes, maybe we could do this as a separate step. PTAL, I made the |
@ed7coyne, I'd like to update this PR with the refactoring changed in #42. What do you think of the following client code?
Or more implicitly cast
|
Looks good, I think fbase.create() should be more explicit it sounds like it would be used to create an object on the firebase backend. |
Let's revisit this with #60 |
Fixes #38
All methods now return a
FirebaseObject
that wrapJsonObject
from ArduinoJson.You can access result with:
/cc @aliafshar @ed7coyne