Refactored Firebase library #42

ed7coyne · 2016-01-12T22:55:16Z

As discussed in issue #41

Refactored to provide result and not depend on class state for error reporting, conform to Google C++ style.

Also broke FirebaseEventStream out from Firebase. When this is connected I believe any other calls (push,fetch,etc..) to the class would break it. For that reason it really seems like this should be its own focused class. Or you need large comments explaining this gotcha.

…eturn type to include status, changed style to match Google Style guide.

ed7coyne · 2016-01-12T22:57:11Z

Also should note this is based on the other pull request so ignore that one here and only accept this one when that one is in.

proppy · 2016-01-13T12:13:31Z

Also broke FirebaseEventStream out from Firebase. When this is connected I believe any other calls (push,fetch,etc..) to the class would break it. For that reason it really seems like this should be its own focused class. Or you need large comments explaining this gotcha.

If we make sendRequest call _http.stop() that should allow the user to do a post() and then resume stream().

As I commented on #41: I'm not sure we should make it easy for the user to do multiple thing at a time with the same client. If they want to do streaming and post update they could create multiple instance of the Firebase object, if they want to reuse the same object they have to call stream() again if they want to resume streaming

The idea was that stream() was at the same level than other HTTP methods like post(), get() etc but long running: #43 try to make it more obvious by decoupling "getting the result" from "making the request".

Ideally I like to converge in a direction where there the only difference between get() and stream() is that you can call read() or data() multiple time after stream()

proppy · 2016-01-13T12:17:00Z

examples/FirebasePush_ESP8266/FirebasePush_ESP8266.ino

-  // handle error.
-  if (fbase.error()) {
+   // add a new entry.
+  FirebaseResultWithMessage push = fbase.push("/logs", "{\".sv\": \"timestamp\"}");


Is that a different type: FirebaseResultWithMessage? I don't see it declared in Firebase.h

Yup, I had it then decided it wasn't worth the complexity to maintain two types. I liked that it was explicit in what returned a body and what didn't.
I will fix it.

ed7coyne · 2016-01-14T01:26:21Z

I did a major change over to a different paradigm. Let me know what you think of this one. This pushes more of the logic into the returned object a FirebaseCall. This allows the optional rawResponse() call or the library to not read the response if the user is not interested in it. It reuses the http object for all calls except the streaming one.

I still kept the streaming impl pretty separate but accessable through the main Firebase interface. It just feels really disjoint to me. It holds the http object for a while, the others are all immediate. It allows multiple reads, it has events.

I am off until next week so we can discuss more then, just wanted to post this up. I am not sure the streaming example works for me either.

proppy · 2016-01-20T19:18:38Z

examples/FirebaseStream_ESP8266/FirebaseStream_ESP8266.ino

-void loop() {
-  if (fbase.error()) {
+void loop() {  
+  static FirebaseEventStream stream = fbase.stream("/bitcoin");


I didn't realize this would force the user to store a static, I think we should find a way that keep the state in the Firebase object so that the user don't have to worry about the lifetime of the result object.

Ya I am still playing with that but it would have to be either a static like this and use return value optimization to allow this to be constructed or to switch over to returning a unique_ptr which is a little ugly but would work like we want.

maybe we could move this as a global?

Give it a shot, I thought something was preventing that from working. I think it was you can't construct the event stream until setup() is called which won't happen if you directly initialize the global.
It isn't really a copyable object with the HTTP connection so you can't create a global then copy this value in.
I think we either need to make it a movable object which is probably the cleanest for the client, or return a unique_ptr<> to the object on the heap. This way is the most standard for c++ but maybe not arduino and looks ungly.

proppy · 2016-01-20T19:24:47Z

I did a major change over to a different paradigm. Let me know what you think of this one. This pushes more of the logic into the returned object a FirebaseCall. This allows the optional rawResponse() call or the library to not read the response if the user is not interested in it. It reuses the http object for all calls except the streaming one.

Made a few more comments, I like the separation of the logic but I'd like to find a way not to push the FirebaseCall type and its lifetype to the user.

I still kept the streaming impl pretty separate but accessable through the main Firebase interface. It just feels really disjoint to me. It holds the http object for a while, the others are all immediate. It allows multiple reads, it has events.

I feel like the streaming impl is a call that can yield multiple result, all the other methods also hold the http object in a way since they don't re-inialize the connection between each call.

proppy · 2016-01-20T21:22:07Z

I feel we should hold on the refactoring until we merge #43: json will likely warrant returning JsonObject or a FirebaseResult wrapper and affect the way we split "making the request", "checking for errors" and "consuming result".

@ed7coyne @aliafshar does that sound OK with you?

This reverts commit 077e060.

ed7coyne · 2016-01-28T04:02:27Z

Updated to reflect changes decided in #41

proppy · 2016-01-28T18:28:23Z

Firebase.h

+
+class FirebaseGetResult : public FirebaseResult {
+ public:
+  static FirebaseGetResult FromError(const String& error_message) {


does this need to be public? it seems it is only being used by the client impl.

We could make them private and make them friend classes. This way allows clients to create tests easily though. I don't feel strongly either way.

proppy · 2016-01-28T18:32:10Z

Firebase.h

+  // True if no error.
+  operator bool() const;
+
+  bool isError() const;


I wonder if we should keep the separate FirebaseError type, with a message() method, etc, and have API method specifics result types compose the generic error and the method specific result.

Sounds good.

proppy · 2016-01-28T18:40:30Z

Just some nits, I'd be happy to send a PR addressing them.

proppy · 2016-01-28T19:18:37Z

Firebase.cpp

+  return url;
+}
+
+class FirebaseCall {


Maybe we should rename that into HTTPRequest since there is not much specific to firebase in this helper (basically just the port and the fingerprint).

firebase: refactor the refactoring

proppy · 2016-01-28T23:40:24Z

Let's merge this once we fix the streaming sample.

proppy · 2016-01-29T10:47:00Z

Firebase.cpp

+FirebaseCall::FirebaseCall(const String& host, const String& auth,
+			   const char* method, const String& path,
+			   const String& data, HTTPClient* http) {
+  if (!http) {


We should probably reuse the client for now (even for stream) and do that in a separate PR (that might be the cause of the crash we see with the stream sample)

firebase: fix stream example

Refactor merge

Refactored Firebase library

ed7coyne added 3 commits January 12, 2016 14:21

Refactor Firebase module, split into more focues subclasses, change r…

0a613cd

…eturn type to include status, changed style to match Google Style guide.

removed FirebaseResultWithMessage consolidated into one for simplicity

d805dc6

Added comments for clarify

217d58f

proppy reviewed Jan 13, 2016
View reviewed changes

ed7coyne added 3 commits January 13, 2016 13:57

removed old reference to FirebaseResultWithMessage

b2438cf

Refactor out FirebaseCall

d5dd303

possibly fix streaming example

23fbbed

ed7coyne added 2 commits January 13, 2016 17:33

Move helper classes below Firebase in Firebase.h

2bf3a4f

Added error message back to streaming imp

2df5bd6

proppy reviewed Jan 20, 2016
View reviewed changes

Add call ids

077e060

proppy mentioned this pull request Jan 27, 2016

Refactor Firebase class, improve api, change style #41

Closed

ed7coyne added 2 commits January 27, 2016 15:19

Revert "Add call ids"

6f65250

This reverts commit 077e060.

Added independant opaque return types for all calls.

33f81ef

Remove unused method declaration.

2ac7ab3

proppy reviewed Jan 28, 2016
View reviewed changes

firebase: refactor the refactoring

357a1d0

proppy mentioned this pull request Jan 28, 2016

firebase: refactor the refactoring ed7coyne/firebase-arduino#2

Merged

Merge pull request #2 from proppy/refactor-12

c65fef7

firebase: refactor the refactoring

proppy mentioned this pull request Jan 29, 2016

calling set() while streaming crashes #48

Closed

proppy reviewed Jan 29, 2016
View reviewed changes

proppy and others added 5 commits January 29, 2016 11:00

firebase: fix stream example

b286b19

Merge pull request #3 from proppy/refactor

34357a9

firebase: fix stream example

merge

f38602f

examples/stream: fix error handling

e8ac1e9

Merge pull request #4 from proppy/refactor-merge-3

caa70e9

Refactor merge

proppy added a commit that referenced this pull request Jan 29, 2016

Merge pull request #42 from ed7coyne/refactor

96f7d8e

Refactored Firebase library

proppy merged commit 96f7d8e into FirebaseExtended:master Jan 29, 2016

proppy mentioned this pull request Feb 1, 2016

initial json support #43

Closed

ed7coyne deleted the refactor branch April 8, 2016 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored Firebase library #42

Refactored Firebase library #42

ed7coyne commented Jan 12, 2016

ed7coyne commented Jan 12, 2016

proppy commented Jan 13, 2016

proppy Jan 13, 2016

ed7coyne Jan 13, 2016

ed7coyne commented Jan 14, 2016

proppy Jan 20, 2016

ed7coyne Jan 20, 2016

proppy Jan 28, 2016

ed7coyne Jan 28, 2016

proppy commented Jan 20, 2016

proppy commented Jan 20, 2016

ed7coyne commented Jan 28, 2016

proppy Jan 28, 2016

ed7coyne Jan 28, 2016

proppy Jan 28, 2016

ed7coyne Jan 28, 2016

proppy commented Jan 28, 2016

proppy Jan 28, 2016

proppy commented Jan 28, 2016

proppy Jan 29, 2016

Refactored Firebase library #42

Refactored Firebase library #42

Conversation

ed7coyne commented Jan 12, 2016

ed7coyne commented Jan 12, 2016

proppy commented Jan 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ed7coyne commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proppy commented Jan 20, 2016

proppy commented Jan 20, 2016

ed7coyne commented Jan 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proppy commented Jan 28, 2016

Choose a reason for hiding this comment

proppy commented Jan 28, 2016

Choose a reason for hiding this comment