altmetric / embiggen

A Ruby library to expand shortened URLs
https://rubygems.org/gems/embiggen
MIT License
124 stars 6 forks source link

[FEAT] Move shortener list to come from a plain file, Closes #14 #15

Closed AvnerCohen closed 8 years ago

AvnerCohen commented 8 years ago

@mudge Following up on the discussion at #14 , This PR adds the logic to load the list of shorteners from a plain file, based, nearly one to one, on your suggestions.

There are 2 additional changes:

  1. I've added .bundle to the ignore file (created if you have a special configuration for the bundler cli, as in my case, and seems like a reasonble default).
  2. Removed the custom any? implementation of include? to use the built in set.include? rather than running the regular expression on for each domain. This makes the test suite run X2:
› for i in {1..10}; do bundle exec rspec | grep Finished ; done
Finished in 0.11635 seconds (files took 0.1828 seconds to load)
Finished in 0.11136 seconds (files took 0.20086 seconds to load)
Finished in 0.11364 seconds (files took 0.20566 seconds to load)
Finished in 0.11202 seconds (files took 0.18655 seconds to load)
Finished in 0.11531 seconds (files took 0.19866 seconds to load)
Finished in 0.11331 seconds (files took 0.19342 seconds to load)
Finished in 0.14938 seconds (files took 0.25272 seconds to load)
Finished in 0.12448 seconds (files took 0.19337 seconds to load)
Finished in 0.12069 seconds (files took 0.2027 seconds to load)
Finished in 0.1156 seconds (files took 0.19347 seconds to load)
› git checkout shorteners_from_file
Switched to branch 'shorteners_from_file'
› for i in {1..10}; do bundle exec rspec | grep Finished ; done
Finished in 0.05276 seconds (files took 0.19224 seconds to load)
Finished in 0.05239 seconds (files took 0.20213 seconds to load)
Finished in 0.05505 seconds (files took 0.18758 seconds to load)
Finished in 0.0529 seconds (files took 0.19548 seconds to load)
Finished in 0.05358 seconds (files took 0.18276 seconds to load)
Finished in 0.05798 seconds (files took 0.19329 seconds to load)
Finished in 0.05503 seconds (files took 0.20827 seconds to load)
Finished in 0.05771 seconds (files took 0.20531 seconds to load)
Finished in 0.05464 seconds (files took 0.20599 seconds to load)
Finished in 0.05595 seconds (files took 0.20314 seconds to load)

I understand there is something obvious I might be missing in terms of why this was added to begin with but considering we just want to match domain names + all test suite is passing, I am not seeing this scenario yet.

AvnerCohen commented 8 years ago

Thanks @mudge for the clarifications.

  1. I have reverted the change for now, I think it's worth exploring, I'll do so in a separate PR though. That being said, without getting into the details of why (I don't know yet..), I would expect the tests to fail on my PR and they didn't. I've now added test that does fail on such edge cases
  2. chomp! vs chomp is fixed, missed that :(

(I'll squash once we are done here)

mudge commented 8 years ago

Small nitpick on the test description but otherwise this looks good.

AvnerCohen commented 8 years ago

squashed and updated - spec title updated as per suggestion.