Skip to content

Do not execute anything if command line is invalid #222

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

Closed
MaxG87 opened this issue Feb 18, 2020 · 3 comments
Closed

Do not execute anything if command line is invalid #222

MaxG87 opened this issue Feb 18, 2020 · 3 comments

Comments

@MaxG87
Copy link

MaxG87 commented Feb 18, 2020

In certain circumstances Fire will execute python code albeit the command line is not valid. Consider the following MWE:

#!/usr/bin/env python

import fire


def yet_another_function(*, argument: bool = False) -> None:
    print(f"Executing yet_another_function() with argument={argument}.")


if __name__ == "__main__":
    fire.Fire({"yaf": yet_another_function})

If I execute it with

./mwe.py yaf yet-another-argument

I get

Executing yet_another_function() with argument=False.
ERROR: Could not consume arg: yet-another-argument
Usage: mwe.py yaf

For detailed information on this command, run:
  mwe.py yaf --help

Note that the function is executed anyways.

In my case this behaviour let a bug manifest in a CI pipeline. I would have preferred to see the step break directly.

@MaxG87
Copy link
Author

MaxG87 commented Feb 20, 2020

I found another MWE.

#!/usr/bin/env python

import fire


def my_fun(arg1, arg2, *, arg3="default"):
    print(arg1, arg2, arg3)


if __name__ == "__main__":
    fire.Fire()

Execution gives:

$ /tmp/mwe.py my_fun arg1 arg2 --arg4 foo
arg1 arg2 default
ERROR: Could not consume arg: --arg4
Usage: mwe.py my_fun arg1 arg2

For detailed information on this command, run:
  mwe.py my_fun arg1 arg2 --help

Again, the function is executed albeit the invocation is quite obviously invalid.

@dbieber
Copy link
Collaborator

dbieber commented Feb 21, 2020

The reason this doesn't work today is that Fire supports method chaining.
Fire has no way of knowing whether --arg4 will be a valid argument to the result of my_fun (which could be a function) until after it executes my_fun.

This is the same issue raised in #168.

As noted there, there may be a potential fix.

The fix would be to require explicit chaining (using a separator, which is "-" by default) when not all the arguments are received, and only allow implicit chaining when all arguments have values.
This would break some commands that are possible today, so we'll need to consider carefully if this change would be worthwhile.

We are also thinking through a "strict mode" for fire, which uses annotations and default values to do type checking. In strict mode, it would be possible to provide better protections against the issue you raise too.

@MaxG87
Copy link
Author

MaxG87 commented Feb 24, 2020

Issue #168 explains this quite nicely. However, I think some improvement on this would be desirable. After all, I am using a CLI framework not only because it is convenient to use, but also because I hope to gain some early error detection.

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