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

Added Rubocop, upgraded Faraday, removed Gemfile.lock. #61

Merged
merged 1 commit into from
Sep 27, 2014

Conversation

dblock
Copy link
Collaborator

@dblock dblock commented Sep 18, 2014

This is a bunch of related things.

  • Removed Gemfile.lock, this is a library it shouldn't rely on fixed locked gems, but only on its declaration in .gemspec - people using hyperclient don't get that lock-in.
  • Added Rubocop, automatic style guide, cleans up common Ruby issues and nobody needs to argue about code style anymore ever.
  • Replaced faraday digest implementation with https://github.com/bhaberer/faraday-digestauth.

@dblock dblock force-pushed the rubocop-faraday-etc branch from b621c4e to 8561589 Compare September 18, 2014 22:07
This was referenced Sep 18, 2014
@dblock dblock mentioned this pull request Sep 26, 2014
@dblock dblock force-pushed the rubocop-faraday-etc branch from 8561589 to ab42327 Compare September 26, 2014 18:36
@@ -1,5 +1,5 @@
require 'faraday'
require_relative 'request/digest_authentication'
require 'faraday/digestauth'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 👎 on this since this gem seems a blatant rip of the actual code at hyperclient. See a47d7db and e443018#diff-05401ebefab536882f566b668412c798L2 vs bhaberer/faraday-digestauth@d41137a

Also, the gem author forgot to copy the tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, actually it's a rip from https://gist.github.com/kapkaev/5088751 :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this doesn't really belong in Hyperclient and is a generically useful Faraday thing - who copies from whom is beyond the point, no? I guess I am trying to say that instead of adding this code to Hyperclient someone (you? :)) should have made a gem out of this. Looks like @bhaberer did just that (and thanks).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do let me know if you want me to undo this, though.

I made a PR into the gem acknowledging where it comes from and porting tests, bhaberer/faraday-digestauth#6.

@bhaberer
Copy link

Yea the person who posted the gist never responded to any messages so I had no idea that this came from someplace else.

I'll merge that pull in a bit, and add @oriolgual as a gem author, if he wants?

I'm fairly amicable to whatever. I just need/ed Faraday to speak digest.

@oriolgual
Copy link
Member

That seems super reasonable, thank you both :) and 👍 on this PR

dblock added a commit that referenced this pull request Sep 27, 2014
Added Rubocop, upgraded Faraday, removed Gemfile.lock.
@dblock dblock merged commit ecaf3a6 into codegram:master Sep 27, 2014
@dblock dblock deleted the rubocop-faraday-etc branch September 27, 2014 15:00
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.

3 participants