Add Slop::Result#fetch #232

giovannibenussi · 2018-06-19T14:16:20Z

Sometimes I want to raise an error when a given key is not present (for avoid typos for example) so this change implements Slop::Result#fetch allows the following:

opts = Slop.parse do |o|
  o.string '-name'
end

ARGV #=> --name giovanni

opts[:name]          # giovanni
opts.fetch(:name) # giovanni

opts.fetch(:namr)      # raises KeyError: key not found: 'namr'
opts.fetch(:last_name) # raises KeyError: key not found: 'last_name'

Slop::Result#fetch returns the value associated to a given key as Slop::Result#[] and Slop::Result#get but raises KeyError if it does not exist.

olleolleolle

Spelling note.

olleolleolle · 2018-06-19T14:19:38Z

lib/slop/result.rb

@@ -20,6 +20,11 @@ def [](flag)
    end
    alias get []

+    # Returns an options value, raises KeyError if the option does not exist.


options => option's

Thanks, fixed here and in Slop::Result#[] description!

olleolleolle · 2018-06-19T14:21:08Z

Hash#fetch implements a few more behaviors, are you interested in those, too?

giovannibenussi · 2018-06-19T14:43:14Z

I take a look at it but I think that is a good option to not allow a default value when a given key does not exists, primarily because you know beforehand all the keys that you'll have and you can also specify default values for those keys. What do you think @olleolleolle?

olleolleolle · 2018-06-19T14:44:24Z

@giovannibenussi I usually raise a more specific error message or specific error class in Hash#fetch's block.

olleolleolle · 2018-06-19T14:44:54Z

(But, this was a question, not a judgement.)

giovannibenussi · 2018-06-19T15:57:00Z

No worries :-) !

I'll take ActiveSupport::OrderedOptions as a reference, where they implement options.attribute and options.attribute!. I was taking a look at the pull request when the bang version was proposed and I think that this comment from @asanghi summarises it pretty good:

secrets.yml is more likely than not to contain passwords/keys to services which might be essential to your application's execution. Without those keys the application is unlikely to work as expected (or crash in unexpected ways). The bang version of the method basically allows us to crash in a more expected way, letting us know that a blank has been encountered when it's not expected. In absence of this, we're more likely to write custom code checking for blanks and handling it with custom error methods (and then possibly erroring out anyway).

For the case of Hash#fetch, I would use a default value when I don't know beforehand all the keys that will be processed and I know how to gracefully handle those cases in a generalized way. Slop is different because you know beforehand all the keys and if one is not set, then you can specify a default value.

Let me know you thoughts :-)

giovannibenussi · 2018-06-19T16:02:19Z

I notice that if you declare an option as not required without a default value and don't set it, then Slop::Result#fetch will raise an error:

opts = Slop.parse do |o|
  o.string '-name'
end

ARGV #=> ''

opts.fetch(:name)      # raises KeyError: key not found: 'name'

I think that in this case then nil should be returned.
I can work on this later!

giovannibenussi · 2018-06-25T13:17:57Z

I made some changes so now Slop::Result#fetch handles the case where a key is not provided and it does not have a default value. I also add a test for this case and the case when a value is not provided but the option has a default value.

olleolleolle · 2018-06-25T13:32:58Z

lib/slop/result.rb

+    def fetch(flag)
+      o = option(flag)
+      if o.nil?
+        raise(KeyError.new(%Q(key not found: '#{clean_key(flag)}')))


can also be expressed as

raise KeyError, %Q(key not found: '#{clean_key(flag)}')

What about:

raise KeyError, "key not found: '#{clean_key(flag)}'"

I think that I was using %Q because at some point I wanted to show an error message like key not found: "key" but it is no longer necessary.

giovannibenussi · 2018-06-25T15:43:35Z

How do you feel about this changes?
They are ok for you? :-)

Regards!

leejarvis · 2018-06-25T15:56:50Z

lib/slop/result.rb

    def [](flag)
      (o = option(flag)) && o.value
    end
    alias get []

+    # Returns an option's value, raises KeyError if the option does not exist.
+    def fetch(flag)
+      o = option(flag)


This could be simpler because we already support the functionality in Result#[]:

def fetch(flag) get(flag) or raise KeyError, "key not found: '#{clean_key(flag)}'" end

Also I don't think the message is clear enough. I would opt for something like "Could not find option with flag '#{flag}'". I don't think the flag needs cleaning either, but I'm not too bothered either way. Also, any reason we shouldn't just re-use Slop::UnknownOption since it basically means the same thing?

get(flag) or raise KeyError, "key not found: '#{clean_key(flag)}'" will throw an exception if the value of the key flag is nil this happens when you don't provide an option and that option does not have a default value as I test in test/result_test.rb:

it "returns nil when an option is not provided and it does not have a default value" do @options.string("--hello") @result.parser.parse %w() assert_equal nil, @result.fetch('hello') end

I tried to remove clean_key(flag) but if you try to make something like @result.fetch("--unexisting") then you'll receive the following error: key not found: '--unexisting'.

As for Slop::UnknownOption, I think that I don't express well enough the purpose of Slop::Result#fetch. The idea of Slop::Result#fetch is the same as [] and get, but with the difference that an exception is thrown if a defined argument is not provided, with the aim of avoid typos. So I think that Slop::UnknownOption is not appropiated here because this exception as far as I understand is raised when you try to fetch an undefined argument.

Related with the previous paragraph, maybe your proposal of "Could not find option with flag '#{flag}'" is not accurate enogh, but I'm open to suggestions anyways :-)

will throw an exception if the value of the key flag is nil this happens when you don't provide an option and that option does not have a default value as I test in test/result_test.rb

Right, good catch!

I tried to remove clean_key(flag) but if you try to make something like @result.fetch("--unexisting") then you'll receive the following error: key not found: '--unexisting'.

Yeah, I think that's okay. As I mentioned though, I don't feel too strongly about it so feel free to keep it. I'd be up for re-using Slop::UnknownOption, though

I'd be up for re-using Slop::UnknownOption, though

I think about it and you're right, so I make the changes for this!

I update the documentation for Slop::UnknownOption also so maybe you want to take a look at it:

Raised when an unknown option is parsed or when trying to fetch an
unexisting option via Slop::Result#fetch.
Suppress with the suppress_errors config option.

leejarvis · 2018-06-25T15:58:01Z

lib/slop/result.rb

      end
    end

+    def clean_key(key)


If you decide to keep this method, please move it to the bottom of the class and mark it as private. It should only be used internally

leejarvis · 2018-06-25T16:00:10Z

test/result_test.rb

+      assert_equal "lee", @result.fetch("--name")
+      assert_equal "bar", @result.fetch("long_option")
+      assert_equal "bar", @result.fetch(:long_option)
+      assert_equal "bar", @result.fetch("--long-option")


I think this is a bit over-tested because it's not just testing Result#fetch, it's also testing Result#get and others. I think the test examples are good though, but all of them only need one assertion without the duplication

You think it's ok to just leave the following two tests:

assert_equal "lee", @result.fetch("name")

and

e = assert_raises(KeyError) { @result.fetch("unexisting") } assert_equal "key not found: 'unexisting'", e.message

I'm not pretty sure if remove "returns the default value of an option when a value is not provided" and "returns nil when an option is not provided and it does not have a default value", because they test edge cases, especially the last one.

I'm not pretty sure if remove "returns the default value of an option when a value is not provided" and "returns nil when an option is not provided and it does not have a default value", because they test edge cases, especially the last one.

I think they should be kept too, yes, especially since they catch the nil issue I mentioned in another comment :)

Cool, I just push those changes :-)

leejarvis · 2018-06-26T14:42:03Z

test/result_test.rb

-      assert_equal "bar", @result.fetch("--long-option")
+    end
+
+    it "raises KeyError when an option does not exists" do


The description needs updating now :)

Good catch, updated!

olleolleolle · 2018-06-26T14:48:37Z

This is a fine addition to Slop!

Collaboration is happening!

Thanks for creating this, together!

leejarvis · 2018-06-26T16:11:22Z

lib/slop/result.rb

+      o = option(flag)
+      if o.nil?
+        cleaned_key = clean_key(flag)
+        raise UnknownOption.new("key not found: '#{cleaned_key}'", "#{cleaned_key}")


One last nitpick I promise :)

I think the error message should have more detail. "key not found" might be confusing in the context of command line options and flags. After all, what is a key? Perhaps we could just re-use option like we did for the other UnknownOption class ("option not found") to clear this up? That might be enough to avoid confusion. What do you think?

Don't worry, I also really want to have this feature as good as possible so don't worry!
If you have any other suggestion I'm open to them :-)

Thanks for your feedback!

leejarvis · 2018-06-26T16:25:14Z

@giovannibenussi Thanks for all your work and patience! This looks really great. Please could you squash all of your commits into one? (or into as many as you'd like if you think they're worth retaining for context). Then we'll get it merged. Thanks again! ✨ 💜

giovannibenussi · 2018-06-26T17:06:56Z

Sure @leejarvis, I'm gonna make it later because I don't have that much experience squashing commits but I'm gonna learn how to do it!

leejarvis · 2018-06-26T17:43:28Z

@giovannibenussi Great! Give me a shout on Twitter at lee_jarvis or if you're on IRC then irc.freenode.org in the #ruby-lang channel as ljarvis and I'll help you go through it if you'd like

olleolleolle · 2018-06-27T07:44:21Z

@giovannibenussi I'm partial to git-extras' git squash command. Documentation link

@leejarvis If you interact with the tiny white triangle on the huge green Merge button, you can pick Squash and Merge, but then you won't get any "Merge Commit", which is a difference, of course. Do you prefer to have Merge Commits?

Huge screenshot

use Slop::Result#clean_key in Slop::Result#option fix typo in Slop::Result#[] and Slop::Result#fetch descriptions handle case when Slop::Result#fetch tries to fetch an option that is not provided and does not have a default value raise Slop::UnknownOption when Slop::Result#fetch tries to fetch an unexisting key set Slop::Result#clean_key method as private remove redundant Slop::Result#fetch tests update description of Slop::Result#fetch test when trying to access an unexisting option update error message when an option is not present on Slop::Result#fetch description of Slop::Result#fetch update expected error message on test for Slop::Result#fetch when an option is not present

giovannibenussi · 2018-06-27T15:33:11Z

Thanks for your help guys, I finally can squash the commits with the help of a great tutorial that I find out!

Thanks a lot for your help and guidance through this new feature, it was nice to work with both of you!

If you have another last-minute change just tell me!
And if you need help with anything I'm open to help you guys :-)

Regards!

leejarvis · 2018-06-27T20:53:25Z

@olleolleolle Thanks, I didn't know of that button addition.

@giovannibenussi Great work! Thanks again for the contribution, I'll get this merged before the end of the week (@olleolleolle feel free to merge and update the changelog if you'd like)

giovannibenussi · 2018-06-27T21:03:24Z

No problem @leejarvis !

Thanks to both of you again! :-)

olleolleolle · 2018-06-27T21:16:08Z

There, CHANGELOG amended.

Update ruby-slop to 4.7.0. pkgsr change: Add "USE_LANGUAGES= # none". v4.7.0 (2019-06-29) ------------------- Features: * Add `Slop::Result#fetch`. It returns the value of given option, or raises an error if given option is not present. [#232](leejarvis/slop#232) ([Giovanni Benussi](https://github.com/giovannibenussi)) * Adding a separator without passing any arguments now creates a separator with the empty string. [#238](leejarvis/slop#238) ([Teemu Matilainen](https://github.com/tmatilai)) Bug fixes * Ensure non-string option types have their flags consumed properly [#241] (Sutou Kouhei)

olleolleolle reviewed Jun 19, 2018

View reviewed changes

olleolleolle reviewed Jun 25, 2018

View reviewed changes

leejarvis reviewed Jun 25, 2018

View reviewed changes

leejarvis reviewed Jun 26, 2018

View reviewed changes

giovannibenussi force-pushed the add-result-fetch branch from 5882f66 to 0b6d52e Compare June 27, 2018 15:24

olleolleolle merged commit 45a66b0 into leejarvis:master Jun 27, 2018

Add Slop::Result#fetch #232

Add Slop::Result#fetch #232

Conversation

giovannibenussi commented Jun 19, 2018

olleolleolle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olleolleolle commented Jun 19, 2018

giovannibenussi commented Jun 19, 2018

olleolleolle commented Jun 19, 2018

olleolleolle commented Jun 19, 2018

giovannibenussi commented Jun 19, 2018

giovannibenussi commented Jun 19, 2018

giovannibenussi commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannibenussi commented Jun 25, 2018

leejarvis Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

giovannibenussi Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olleolleolle commented Jun 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejarvis commented Jun 26, 2018

giovannibenussi commented Jun 26, 2018

leejarvis commented Jun 26, 2018 • edited Loading

olleolleolle commented Jun 27, 2018 • edited Loading

Huge screenshot

giovannibenussi commented Jun 27, 2018

leejarvis commented Jun 27, 2018

giovannibenussi commented Jun 27, 2018

olleolleolle commented Jun 27, 2018

leejarvis Jun 25, 2018 •

edited

Loading

giovannibenussi Jun 25, 2018 •

edited

Loading

leejarvis commented Jun 26, 2018 •

edited

Loading

olleolleolle commented Jun 27, 2018 •

edited

Loading