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

Add OptionParser integration #18

Merged
merged 17 commits into from Oct 29, 2018

Conversation

Projects
None yet
2 participants
@jastkand
Contributor

jastkand commented Oct 13, 2018

@palkan this PR addresses this feature #17. Any comments and suggestions?

I've extracted the serialize_val method to Anyway::Ext::ValueSerializer module. Probably, it's better to extract it to an Anyway::Utils class. This way we won't have to include the module to the Env and Config classes and call it using the Anyway::Utils.serialize_val(value). What do you think?

@palkan

Great work! Thanks!

Left some comments.

I was thinking about the initial proposal (form #17) and found it a little bit confusing (yeah, my mistake): mixing whitelisting and descriptions looks strange, and API is not clear.

Also, I think it's better to provide a blacklisting API, not whitelisting.

My new proposal is to add two separate methods to configure option parser:

class MyConfig < Anyway::Config
  attr_config :host, :log_level, :concurrency, server_args: {}

  # specify which options shouldn't be handle by option parser
  ignore_options :server_args

  # provide description
  describe_options(
    concurrency: "number of threads to use"
  )
end

Another idea is to add DSL to extend parser (instead of overriding build_option_parser):

class MyConfig < Anyway::Config
  # ...
  extend_options do |parser|
    parser.banner = "mycli [options]"
    parser.on_tail "-h", "--help" do
      # ...
    end
  end
end

NOTE: we should also take into account inheritance: how do we want to handle inherited parsers attributes/extensions? I mean:

class OneConfig
  attr_config :a, :b, :c
  ignore_options :a, :b
end

class SubConfig < OneConfig
  # adds new parameters
  attr_config :x, :y
  ignore_options :x
end

@jastkand ^ What do you think?


module Anyway
module Ext
# Extend Object through refinements

This comment has been minimized.

@palkan

This comment has been minimized.

@palkan

palkan Oct 14, 2018

Owner

Btw, we can implement serializer through refinements (as String extension, i.e. val.serialize).

This comment has been minimized.

@jastkand

jastkand Oct 15, 2018

Contributor

Copy-paste issue. Yes, I'll update the implementation to use refinements.

@@ -135,6 +158,18 @@ def load_from_env(config)
config
end

def option_parser
build_option_parser if @option_parser.nil?

This comment has been minimized.

@palkan

palkan Oct 14, 2018

Owner

Let's make it simpler: @option_parser ||= build_option_parser

def parse_options!(options)
option_parser.parse!(options)
rescue OptionParser::InvalidOption
# NOTE: Do not fail when unknown arguments were given

This comment has been minimized.

@palkan

palkan Oct 14, 2018

Owner

Why do we want to ignore unknown arguments?

This comment has been minimized.

@jastkand

jastkand Oct 15, 2018

Contributor

When unknown arguments are passes to parse_options! method, like:

class MyConfig < Anyway::Config
  attr_config :host, :port
end

...

config.parse_option!(%w[--host localhost --port 3000 --log-level debug])

the mehtod call will raise an OptionParser::InvalidOption exception with the message "unknown argument '--log-level'"`. I don't think it's a good behavior to raise an exception in this case.

This comment has been minimized.

@palkan

palkan Oct 25, 2018

Owner

That's a normal behaviour to raise an exception when unknown argument is passed instead of silently ignoring.

If a developers don't want to raise exceptions they could rescues them themselves

This comment has been minimized.

@palkan

palkan Oct 25, 2018

Owner

@jastkand What about this one?

This comment has been minimized.

@jastkand

jastkand Oct 26, 2018

Contributor

If a developers don't want to raise exceptions they could rescues them themselves

That's fair.

Anyway, let me confirm if I correctly understand the behavior which you expect. Let's say we have a class:

class MyConfig < Anyway::Config
  attr_config :host, :port, :log_level

  ignore_options :log_level
end

Should it raise the OptionParser::InvalidOption exception when I call:

config.parse_option!(%w[--host localhost --port 3000 --log-level debug])

This comment has been minimized.

@palkan

palkan Oct 26, 2018

Owner

Yep, it should raise an exception

def build_option_parser
require 'optparse'
@option_parser ||= OptionParser.new do |opts|
self.class.option_parser_attributes.each do |(key, options)|

This comment has been minimized.

@palkan

palkan Oct 14, 2018

Owner

What is options? Isn't option_parser_attributes an array of key?

This comment has been minimized.

@jastkand

jastkand Oct 15, 2018

Contributor

It's a leftover from the previous implementation. Initially, I wanted to store the option_parser_attributes as a hash, like

{
  host: { description: 'Host name' },
  ...
}

Then I've changed my mind but didn't update the code accordingly.

new_descriptions.stringify_keys!
@option_parser_descriptions.merge! new_descriptions

new_keys = (args + new_descriptions.keys) - @option_parser_attributes

This comment has been minimized.

@palkan

palkan Oct 14, 2018

Owner

@option_parser_attributes = @option_parser_attributes | (args + new_descriptions.keys)?

@@ -155,5 +190,24 @@ def parse_yml(path)
YAML.load_file(path)
end
end

def build_option_parser

This comment has been minimized.

@palkan

palkan Oct 14, 2018

Owner

What about extracting this functionality into its own module?
For example, OptionParserBuilder.call(options) where options is a Hash of options names as keys and descriptions (or nil-s) as values?

Then we only need to generate a proper options Hash in the config itself.

That would make testing a little bit easier.

This comment has been minimized.

@jastkand

jastkand Oct 15, 2018

Contributor

Sounds good

@jastkand

This comment has been minimized.

Contributor

jastkand commented Oct 15, 2018

@palkan Thanks for your response! I like the blacklisting API. The ignore_options and describe_options methods will make the usage of that feature more explicit and flexible.

Another idea is to add DSL to extend parser (instead of overriding build_option_parser)

Sounds good!

NOTE: we should also take into account inheritance: how do we want to handle inherited parsers attributes/extensions?

I think should behave the same way as attr_config works now (As I see the inherited class extends the configuration of parent class). Otherwise, it will be confusing for gem users that different methods are working differently.

P. S. I'll go through the comments later.

@palkan

This comment has been minimized.

Owner

palkan commented Oct 16, 2018

I think should behave the same way as attr_config works now (As I see the inherited class extends the configuration of parent class)

Looks like a bug :) config_attributes are not inherited: we initialize a new array for attribute here for every subclass.
Their should be smth like:

@config_attributes ||= superclass <= Anyway::Config ? superclass.config_attributes.dup : []

The only thing that's inherited is attr_accessor.

@jastkand jastkand changed the title from Add OptionParser integration to [WIP] Add OptionParser integration Oct 18, 2018

@@ -27,6 +27,8 @@ def stringify_keys!
value.stringify_keys! if value.is_a?(::Hash)
self[key.to_s] = value
end

self

This comment has been minimized.

@jastkand

jastkand Oct 20, 2018

Contributor

Probably, it's better to extract this bug-fix to a separate PR?

@jastkand jastkand changed the title from [WIP] Add OptionParser integration to Add OptionParser integration Oct 20, 2018

@jastkand

This comment has been minimized.

Contributor

jastkand commented Oct 25, 2018

Hey, @palkan. It's ready for review.

@palkan

Let's update Readme and Changelog and we're done

def parse_options!(options)
option_parser.parse!(options)
rescue OptionParser::InvalidOption
# NOTE: Do not fail when unknown arguments were given

This comment has been minimized.

@palkan

palkan Oct 25, 2018

Owner

That's a normal behaviour to raise an exception when unknown argument is passed instead of silently ignoring.

If a developers don't want to raise exceptions they could rescues them themselves

def parse_options!(options)
option_parser.parse!(options)
rescue OptionParser::InvalidOption
# NOTE: Do not fail when unknown arguments were given

This comment has been minimized.

@palkan

palkan Oct 25, 2018

Owner

@jastkand What about this one?

@jastkand jastkand force-pushed the jastkand:add-option-parser-support branch from d4815c3 to 3389bec Oct 28, 2018

@jastkand

This comment has been minimized.

Contributor

jastkand commented Oct 28, 2018

@palkan, I've updated a readme and changelog and removed the rescue from parse_options! method.

@palkan

palkan approved these changes Oct 29, 2018

@palkan palkan merged commit b32a67b into palkan:master Oct 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@palkan

This comment has been minimized.

Owner

palkan commented Oct 29, 2018

Thanks for your help!

@palkan palkan referenced this pull request Oct 29, 2018

Closed

Add OptionParse integration #17

@jastkand jastkand deleted the jastkand:add-option-parser-support branch Oct 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment