search

jugyo / earthquake

Twitter terminal client with streaming API support.
MIT License
661 stars 94 forks source link

Depend on slop 3.0 or above #135

Closed eitoball closed 12 years ago

eitoball commented 12 years ago

This PR should fix issue #134, but it is really ugly. The ugly part is to work around a bug in slop. For example, 

opts = Slop.new {on: 'no-logo'}
opts.parse(%w{--no-logo})
opts.to_hash #=> {:"no-logo" => false}

I think that this should be true. Maybe, slot 2.x returns true?

Anyway, this command parsing logic should be refactored like discussed in #121.

jugyo commented 12 years ago

Cool!!

leejarvis commented 11 years ago

This should be fixed in https://github.com/injekt/slop/issues/86 you should be able to remove any hacks now once I've released a new bugfix version. Sorry for the issue, please let me know in the Slop tracker if you find anything weird happening, or of course if there's anything you'd like integrated to help earthquake.

leejarvis commented 11 years ago

I've also noticed that you've bumped to Slop 3.0 without fixing any of the incompatibilities with 2.0. Mostly here from what I can see. Slop 3 doesn't support the true option sent to the command, instead you should use :command= to signify that option expects an argument, or the :argument => true config option.

eitoball commented 11 years ago

@injekt thanks for fix and suggestion. I will try to write and submit a patch based on your suggestion this weekend.