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

Depend on slop 3.0 or above #135

Merged
merged 1 commit into from
Aug 7, 2012
Merged

Depend on slop 3.0 or above #135

merged 1 commit into from
Aug 7, 2012

Conversation

eitoball
Copy link
Contributor

@eitoball eitoball commented Aug 7, 2012

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 pushed a commit that referenced this pull request Aug 7, 2012
Depend on slop 3.0 or above
@jugyo jugyo merged commit 6e1ac57 into jugyo:master Aug 7, 2012
@jugyo
Copy link
Owner

jugyo commented Aug 7, 2012

Cool!!

@leejarvis
Copy link

This should be fixed in leejarvis/slop#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
Copy link

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
Copy link
Contributor Author

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

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