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

Allow arguments to be hardcoded strings with - and = characters #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexB52
Copy link

@AlexB52 AlexB52 commented Feb 20, 2024

Describe the change

This pull request allows :argument option to be a hardcoded string representing a command that can contain args and env variables.

Note: Please advise if there is another way to define an argument to would allow a hardcoded string with special characters like = and -

Ex: my-cli --notify 'bundle exec rake TEST=<test>'

module Retest
  class Options
    include TTY::Option

    argument :command do
      optional
      desc <<~EOS
      The test command to rerun when a file changes.
      Use <test> or <changed> placeholders to tell retest where to reference the matching spec or the changed file in the command.
      EOS
    end

    flag :notify do
      long "--notify"
      desc "Play a sound when specs pass or fail (macOS only)"
    end
  end
end

Why are we doing this?

I have a gem trying to parse a string like this 'my-command FILE=<test>' as an argument and currently fails to parse the command properly. TTY::Option returns nil when a = or - is used in the string.

Any related context as to why is this is a desirable change.
Here is the issue: AlexB52/retest#81

Benefits

Meta: We can pass a hardcoded string as an argument using ENV variables and flags.

Drawbacks

  • We could have an arg matched as multiple parameter types if not tested properly.
  • The regex used to identify whether it's hardcoded command look a bit weak as we're only checking for the presence of spaces in the string.

Requirements

  • Tests written & passing locally?
  • Code style checked?
  • Rebased with master branch?
  • Documentation updated?
  • Changelog updated?

@piotrmurach
Copy link
Owner

Hi Alexandre 👋

Thanks for using the tty-option in your project and reporting this issue. Your project looks cool.

I'd encourage you to open an issue where a discussion can follow on the best way to approach this problem.

The suggested change is not the way to solve this. At least with a cursory glance. It may solve your specific case but I'm only interested in generic solutions that better experience for everyone.

Relying on the presence of whitespace is ambiguous. We need clear criteria to disambiguate this from other inputs including keywords. So any change would require adding tests that mix all types of inputs. It is especially key to ensure that argument inputs work for any arity. Fundamentally, we need to first answer what is the difference between a keyword and an argument that looks like a keyword. How would we tell the parser what we mean by rake TEST=<test> in different scenarios:

my-cli "rake TEST=<test>"
my-cli --foo "rake TEST=<test>"
my-cli --foo="rake TEST=<test>"
my-cli foo="rake TEST=<test>"

The problem gets even more thorny because keywords can appear anywhere, e.i. before arguments. Ideally, we would modify a current regex for one or more parameter types to handle this.

@AlexB52
Copy link
Author

AlexB52 commented Feb 20, 2024

Hi @piotrmurach

I'd encourage you to open an issue where a discussion can follow on the best way to approach this problem.
The suggested change is not the way to solve this. At least with a cursory glance. It may solve your specific case but I'm only interested in generic solutions that better experience for everyone.

That makes perfect sense and I can feel the weakness of the implementation. I will create an issue instead.

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.

2 participants