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

flag-like options in Slop#to_hash object (take2) #59

Closed
wants to merge 3 commits into from

Conversation

no6v
Copy link
Contributor

@no6v no6v commented Feb 29, 2012

I reimplemented (#58) by tweaking Slop::Option#value.
Saving a flag value during processing options (Slop#process_item),
and return the value immediately if the @value exists and it is true, false or nil.
One more assertion tweaked as well.

@leejarvis
Copy link
Owner

I'm still not entirely sure about this. It's also a backward incompatible change which means according to semver (which Slop follows), this can't be merged until Slop v4. I'm going to leave it open for more discussion, though. Thanks for putting this work in

@cmol
Copy link

cmol commented Apr 18, 2012

I'm in for merging this. It seems strange that you can't get the true/false value from .to_hash, like in the older versions

@lolmaus
Copy link
Contributor

lolmaus commented Apr 25, 2012

Have come here to post an issue about not being able to retrieve from .to_hash whether argumentless parameters have been set or not.

I vote that the values of argumentless parameters in .ho_hash should be true for specified parameters and false/nil for not specified ones.

@leejarvis
Copy link
Owner

Thanks for chiming in guys. I'm +1 for this, but it is a backwards incompatible change. It'll have to wait for Slop 4.0

@cmol
Copy link

cmol commented Apr 26, 2012

Great, thanks for listening :) But wasn't this the case in older versions? I know I used it, because it broke when i updated my gems ;) But still, great!

@leejarvis
Copy link
Owner

@cmol Yup that was the case in older versions, and I think I was probably wrong trying to change it. This could perhaps go in as a bugfix instead.

leejarvis pushed a commit that referenced this pull request Apr 30, 2012
@leejarvis
Copy link
Owner

Closing this. It's now merged into master and will be available in 3.2.0 as a bugfix

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.

4 participants