search

chrisribe / ti-firebase

Appcelerator / Titanium module project to build and use the google firebase SDK
Other
33 stars 14 forks source link

Refactor project #6

Closed hansemannn closed 8 years ago

hansemannn commented 8 years ago

Please do not merge this time. It is untested, yet. @m1ga and some others asked for this changes, so here they are. Please go ahead and leave comments, thanks!

hansemannn commented 8 years ago

@chrisribe PR is ready, @m1ga tested it today and probably can give some feedback.

m1ga commented 8 years ago

Thank you @hansemann! With these changes I was able to compile and run the module without any missing architecture errors (which happened without the changes).

chrisribe commented 8 years ago

Wow thanks hansemannn, looks much better did not know " ENSURE_ARG_OR_NIL_FOR_KEY" existed nice and clean. Will try and test the two error cases #1 and #2 maybe tomorrow.

If all is well will merge the pull request to master !

hansemannn commented 8 years ago

I just resolved the merge issues coming from your recent commit. And I can already predict that this will cause problems, not only because the delegate - (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)userInfo fetchCompletionHandler:(void (^)(UIBackgroundFetchResult))completionHandler cannot be called directly. We might consider to leave my version as a fork.

chrisribe commented 8 years ago

I would not worry about that code... I should have not committed that to master sorry. I will move this to a test branch so it does not get in the way...

chrisribe commented 8 years ago

Your changes where merged... Sorry for the new pull request I had to fix some issues and am a novice when it comes to git pull, merge and pull requests...

Question: When I fix a pull request, should I directly merge it to master (via desktop) or create a new fixed pull request... Or something else ? Sorry more a svn guy.