Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructuring the Gem #85

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

EdwinRozario
Copy link

@EdwinRozario EdwinRozario commented Jan 29, 2018

  • Client class moved to a separate file.
  • New class Request created to handle http interactions with firebase.

Edwin Rozario added 6 commits January 26, 2018 10:26
- Added a request class for firebase
- Fixed specs for the above changes
- Defined Firebase::Database class to Replace CLient later
- Rubocop fixes
@vincentwoo
Copy link
Collaborator

Hi Edwin, sorry about the delay. I think that my problem with this PR is that it does too much for the benefit provided. If you're interested in adding support for additional Firebase products, I'd rather look at specific PRs for that rather than a general overhaul of the entire gem. For instance, changes to the new hash syntax will break old Ruby compatibility. It's fine to do that, but we're not getting much benefit out of doing that right now.

@EdwinRozario
Copy link
Author

@vincentwoo The reason i have separated classes is to make future addition of other Firebase services easier.

@vincentwoo
Copy link
Collaborator

I know, but unless we have an active plan to do that work, we are basically asking all our downstream users to refactor their code for no gain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants