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

Bug: Launchy#open does not support { :debug => true } #63

Closed
schmich opened this issue Apr 23, 2013 · 5 comments
Closed

Bug: Launchy#open does not support { :debug => true } #63

schmich opened this issue Apr 23, 2013 · 5 comments
Milestone

Comments

@schmich
Copy link
Contributor

schmich commented Apr 23, 2013

While investigating #62, I noticed the API around Launchy#open is inconsistent and yields unexpected results:

  1. Specifying :debug => true does not enable debug output, you must instead specify :debug => 'true' (string vs. bool). In comparison, :dry_run => true does work, as does :dry_run => 'true'.

  2. The following prints [false, true], but I would expect it to print [false, false].

    ENV['LAUNCHY_DEBUG'] = 'true'
    ENV['LAUNCHY_DRY_RUN'] = 'true'
    
    p [Launchy.debug?.nil?, Launchy.dry_run?.nil?]

    The problem is Launchy#debug? checks the environment every time while Launchy#dry_run? does not.

  3. The following prints ["true", true, "true"], but I would expect it to print [true, true, true].

    Launchy.extract_global_options(:dry_run => 'true')
    d1 = Launchy.dry_run?
    
    Launchy.extract_global_options(:dry_run => true)
    d2 = Launchy.dry_run?
    
    ENV['LAUNCHY_DRY_RUN'] = 'true'
    Launchy.extract_global_options({})
    d3 = Launchy.dry_run?
    
    p [d1, d2, d3]

    The issue here is that Launchy#dry_run? can return a different type depending on how it was set. It should be consistent and always return a bool regardless of how it was set.

This all makes the API tough to work with, so I'll do the following to fix it while trying to maintain compatibility with existing clients:

  1. Support both true and 'true' for the :debug and :dry_run options.
  2. Always have Launchy#dry_run? check the environment just like Launchy#debug? does.
  3. Launchy#dry_run? and Launchy#debug? will always return boolean values, never strings. This should not affect users doing if Launchy.dry_run? then ... end, but could affect users inspecting the return value on a deeper level (e.g. checking the class). I seriously doubt anyone actually does this, so I think this change is fine.

Let me know if this sounds reasonable and I'll make the changes.

Chris

@copiousfreetime
Copy link
Owner

Sure sounds good to me. I'm happy to have consistency in the return values so long as the truthiness or falsiness isn't affected.

Thanks!

@copiousfreetime
Copy link
Owner

Would you like a release now that this is merged, or can I wait until after #62 has a pull request too?

@schmich
Copy link
Contributor Author

schmich commented Apr 25, 2013

Thanks for the quick turnaround. Waiting for #62 is fine. I hope to have that fixed in the next week or so.

@copiousfreetime
Copy link
Owner

Sounds good, I'll wait for #62 then.

@copiousfreetime
Copy link
Owner

Version 2.4.0 has been released including this fix.

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

No branches or pull requests

2 participants