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

Arguments are not working with 'textual run' #1064

Closed
jspv opened this issue Oct 31, 2022 · 9 comments
Closed

Arguments are not working with 'textual run' #1064

jspv opened this issue Oct 31, 2022 · 9 comments

Comments

@jspv
Copy link
Contributor

jspv commented Oct 31, 2022

Per textual run --help:

  If you are running a file and want to pass command line arguments, wrap the
  filename and arguments in quotes:

      textual run "foo.py arg --option"

This is not working. Using the following sample.py:

from textual.app import App, ComposeResult
from textual.widgets import Static
from sys import argv

class MyApp(App):
    def __init__(self, greeting: str="default") -> None:
        self.greeting = greeting
        super().__init__()

    def compose(self) -> ComposeResult:
        yield Static(f"{self.greeting}")

if __name__ == "__main__":
    if len(argv) > 1:
        app = MyApp(argv[1])
    else:
        app = MyApp()
    app.run()

When run via:
python ./sample.py foo
it returns a textual window with foo

When run via:
textual run "./sample.py foo"
it returns a textual window with default

@davep
Copy link
Contributor

davep commented Oct 31, 2022

The issue here is the bit of code towards the end:

if __name__ == "__main__":
    if len(argv) > 1:
        app = MyApp(argv[1])
    else:
        app = MyApp()
    app.run()

The textual run command isn't running that code. When presented with your code, textual run, not seeing a variable called app at the top level (which is fine), goes looking for a class in your module that inherits from App; it then creates an instance of that and .run()s it.

However, even if you were to do that, I suspect it won't have the effect you're hoping for. As you may imagine, if you're going to textual run your application argv will end up looking quite different; at the moment anyway. I'm going to dig into this a little more and see if there might be a better way of doing things.

But there is a different approach you can take. Rather than using textual run to run your app, you can run it just as normal, with python, but have TEXTUAL=devtools in your environment. That gives all the benefits of a textual run without needing to use textual run.

So, for example, in one terminal you can do:

$ textual console

to get the developer console up and running, and then in another do something like (depending on shell):

$ export TEXTUAL=devtools python ./sample.py. foo

and everything should work as you were hoping.

Let me know how you get on and if this solves the issue for you.

davep added a commit to davep/textual that referenced this issue Oct 31, 2022
A `textual run` already made an attempt to monkey-patch `argv` so that the
application being run would not see an `argv` any differently to how it
would if it had been run "vanilla". However, it could be caught out by code
like:

    from sys import argv

    ...

    do_something_with( argv )

beforehand it was okay with:

    import sys

    ...

    do_something_with( sys.argv )

With this change it should work both ways.

Somewhat related to Textualize#1064.
@jspv
Copy link
Contributor Author

jspv commented Oct 31, 2022

My code was simply for example to simulate passing arguments to a script via command line (which would start execution at the global/main scope.) As the textual run help shows support for arguments being passed, is there a different way of doing it?

Setting the TEXTUAL environment variable does indeed allow the console to work without textual run thank you.

Looking at _import_app.py, it appears that the code extracts out the arguments with the shlex.split:

   import_name, *argv = shlex.split(import_name, posix=not WINDOWS)

And then assigns them into the global_vars, but only if sys is one of the returned global_variable from runpy.run_path (which from my testing never seems to get returned in the global_vars)

        if "sys" in global_vars:
            global_vars["sys"].argv = [path, *argv]

It then appears that nothing is ever done with the "sys" key regardless. Perhaps argument handling is still a work-in-progress?

@willmcgugan
Copy link
Collaborator

There's a fix for this in master.

@davep
Copy link
Contributor

davep commented Oct 31, 2022

@jspv Yup, as Will mentions, about 90 minutes before you mentioned it, we did #1070 which should address this. :-)

@davep davep closed this as completed Oct 31, 2022
@github-actions
Copy link

Did we solve your problem?

Glad we could help!

@jspv
Copy link
Contributor Author

jspv commented Oct 31, 2022

Thanks for the quick response. This is still not solving my concern on what is the right way to get arguments in to the app from textual run. From issue 1030, it looked like it was better to pass arguments in was via __init__, but this clearly breaks textual run which only passes them via sys.argv and will break the __init__ call unless logic is added for that particular situation.

What I would expect would be for both python ./sample.py foobar and textual run "./sample.py foobar" to have the exact same result. For me to get this, I need to use logic like below which has to evaluate both methods which seems overly complex. Would it be better that when textual run is called using the explicit python script like textual run "./filename.py argument" to have runpy.run_path execute main just like python does instead of searching for the object?

from textual.app import App, ComposeResult
from textual.widgets import Static
from sys import argv

class MyApp(App):
    def __init__(self, args=None) -> None:
        if args is None:
            if argv is not None:
                self.args = argv
            else:
                self.args = "nothing passed"
        else:
            self.args = args
        super().__init__()

    def compose(self) -> ComposeResult:
        yield Static(f"{self.args}")

if __name__ == "__main__":
    MyApp(argv).run()

@davep
Copy link
Contributor

davep commented Oct 31, 2022

I suspect there's a couple of things at play here, so I'll try as best as I can to explain things exactly how I see them and how I'd document them, so do forgive me if I state the very obvious as I do this.

So, first up, as I understood it, #1030 was very much a question of "what is the best way to pass arguments into an instance of my application class"; while it did also include getting arguments from the command line, that seemed to be more of an implementation detail for a developer. So, to take the example I used over there, this code:

from textual.app import App, ComposeResult
from textual.widgets import Static

class Greetings(App[None]):

    def __init__(self, greeting: str="Hello", to_greet: str="World") -> None:
        self.greeting = greeting
        self.to_greet = to_greet
        super().__init__()

    def compose(self) -> ComposeResult:
        yield Static(f"{self.greeting}, {self.to_greet}")

if __name__=="__main__":
    Greetings().run()
    Greetings(to_greet="davep").run()
    Greetings("Well hello", "there").run()

runs 3 different instances of my app, one after the other, passing different values to it. In this case I'm hard-coding them in the code itself, but I could just have easily pulled them out of argv, or used argparse, etc. Likewise, I could have loaded them from some external configuration file, or pulled them out of environment variables. Where the values for the arguments come from is a detail that Textual isn't involved in; the Textual thing here is "what's the intended way of feeding arguments to a fresh instance of an app".

Now, so far, this is all expecting that the code will be run as python my_app.py (or whatever the app code is called), or perhaps even the code is shebanging python -- that all still works because the code will be the __main__ code.

So then we get to textual run. As I'm sure you know, textual run wouldn't be the normal way you'd run your app. The idea is that you'd only be using textual run for development and testing purposes. So this seems to be where you're coming from with your question. As mentioned earlier, using textual run isn't going to work well if you're using __name__-guarded code to pull values from outside of your app, and feed them into an instance of your app.

In this case I see you having a couple of options, both of each really come down to your own design choices. The first is that, if your app is going to be totally driven by values coming off the command line, it might make more sense to place the argv-consuming logic in your own app class. That way it doesn't matter how your app gets instantiated, all that logic is contained in the app code itself.

The other option, as mentioned earlier, would be to code your app as if it were being run standalone, without being run using textual run, and instead run it with TEXTUAL=devtools in the environment. That way you get the benefits of a textual run while also having your __main__ code executed.

Admittedly I'm fairly fresh to this, so perhaps @willmcgugan will clarify if I'm off a little here, but from my own perspective I view textual run as a handy quick test helper for some kinds of apps, but if I were testing something less trivial, I'd probably not textual run -- exactly for the reasons you've highlighted -- and instead would run my app as it will be run in deployment but with TEXTUAL=devtools in the environment so I can hook up to the debug console, etc.

In summary:

What I would expect would be for both python ./sample.py foobar and textual run "./sample.py foobar" to have the exact same result.

They kind of can't because the former will be running the code that's guarded by if __name__ == "__main__":, whereas the latter is asking textual to create an instance of your app and to .run() it. There is the option to not do the former guarded by if __name__ == "__main__": and as long as you name the instance app then textual run should do the right thing, but personally I'd caution against that.

For what you want to do, I'd say that the textual run equivalent of this:

$ python ./sample.py foobar

is this:

$ TEXTUAL=devtools python ./sample.py foobar

Apologies if that rambles a wee bit, I was hoping to cover all the options. Does that help?

@jspv
Copy link
Contributor Author

jspv commented Oct 31, 2022

Thanks, I truly appreciate the discussion. It would be really good to add TEXTUAL=devtools to the documentation, I agree that is probably the best way to debug if the code has any logic outside of the App instance.

I think the other thing that I found confusing is the output of textual run --help which implies strongly (at least to me) that textual run will pass the arguments via command line to foo.py file itself.

If you are running a file and want to pass command line arguments, wrap the filename and arguments in quotes:

      textual run "foo.py arg --option"

"If you are running a file and want to pass command line arguments" - I think could be easily interpreted that it would call the code the same way as it does when running the file from the command line (e.g. the fairly typical model when running a file that execution starts with __main__). Perhaps being explicit that textual run will only run the app instance and not any surrounding code and that any command line arguments will be available to the app instance via sys.argv" in the help would be clearer. (or maybe I'm just a bit dense :-)

Again, the TEXTUAL=devtools does seem to be the most flexible, any reason to not do this all the time? Is there any particular benefit from running textual console?

@davep
Copy link
Contributor

davep commented Oct 31, 2022

I would imagine that most people will find out about textual run from the docs, where it does give details on how the app is run -- but I take your point that perhaps the help output might give the wrong impression. I suspect there may be some assumption going on in the docs too that folk will appreciate the implications of having the conventional __main__ guard in there.

I'll make a note about ensuring that the environment variable gets clearly mentioned in the docs.

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

3 participants