Skip to content

Conversation

@ashahab-altiscale
Copy link
Contributor

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should tell OptionParser to expect an Integer here.

opts.on('-t',
        '--times NUM_TIMES',
        Integer,
        'Times to run script, default 1.') do |times|
  settings.times = times
end

@raviprak-altiscale
Copy link

Sorry I noticed you comment late Tucker. I've added it in now. Could you please review and merge the branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think that was actually an incomplete review before. The reason to tell optparser to expect an integer is actually to simplify this. With optparser ensuring the correct data type, you can now do this:

settings.times.times do |i|
  logger.debug "Run: #{i}"
  ...snip...
end

It ends up being simpler an somewhat more readable (although settings.times.times does leave a bit to be desired). This is non blocking though. If you'd prefer to move forward without it, please self merge.

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.

5 participants