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

Expand globs in arguments on Windows? #1096

Closed
njsmith opened this issue Aug 24, 2018 · 9 comments · Fixed by #1830
Closed

Expand globs in arguments on Windows? #1096

njsmith opened this issue Aug 24, 2018 · 9 comments · Fixed by #1830
Labels
windows Issues pertaining to the Windows environment
Milestone

Comments

@njsmith
Copy link

njsmith commented Aug 24, 2018

I'm using a variadic Path argument:

@click.command()
@click.argument(
    "paths", nargs=-1, type=click.Path(exists=True, dir_okay=False)
)
def whatever(paths)
   ...

When I run myprogram.py *.txt, then on unix of course it works, because the shell expands on the glob. But on Windows, glob expansion is the responsibility of the program being invoked, and click doesn't appear to do it – even in a case like this where it has all the information needed to tell that glob expansion is appropriate. And click kind of has to do it, because click wants to validate the paths, and this has to happen after glob expansion – if I pass *.txt, it says Error: Invalid value for "paths": Path "*.txt" does not exist., and my code doesn't even get invoked, so I can't even fix up the globs myself, except by disabling all of click's validation and then doing it all myself.

@davidism
Copy link
Member

  • Should it be Click's (built-in) job to make up for cmd not having globs?
  • How can we safely tell that a name is a glob? There are other patterns besides *.
  • What if the name comes from Bash and *.txt was actually the name of the file after expansion?
  • Do we also want to expand ~?
  • If we add glob expansion to parameters, do we also need to do it for env vars on Unix too? For example, if a parameter has an env var NAME="*.txt", no expansion is done, Click currently expects you to handle that.

My first reaction is that this shouldn't be done by Click, although it should be possible to subclass Argument.process_value() or Path to work with globs.

@ThiefMaster
Copy link
Member

What if the name comes from Bash and *.txt was actually the name of the file after expansion?

IMHO such behavior would have to be completely disabled on non-Windows systems.

Maybe the least strange/error-prone solution would be to have a windows_expand_globs argument that can be set to True if you want glob expansion on Windows.

@njsmith
Copy link
Author

njsmith commented Aug 24, 2018

Should it be Click's (built-in) job to make up for cmd not having globs?

The Windows convention that it is every program's job to make up for cmd not having globs, yes. "Annoying thing that every cli app has to do" seems like it falls within click's scope.

How can we safely tell that a name is a glob? There are other patterns besides *.

I believe in traditional Windows globbing the metacharacters are *, ? only. The python glob module also does []. You probably can't exactly match what, like, MSVC would do, but that's ok, it's Windows, it's not your fault and no one expects everything to work consistently.

What if the name comes from Bash and *.txt was actually the name of the file after expansion?

As @ThiefMaster notes, you'd only enable enable this on Windows. But what if someone is using mingw bash on Windows? Well, then you might double-expand, just like every other traditional Windows cli program would in the same situation.

Do we also want to expand ~?

That would be an interesting option for File and Path arguments yeah, though it's more of a Unix thing so the history is a bit different. I can see a good argument for doing expanduser by default on File and Path arguments, with a switch available to turn it off.

If we add glob expansion to parameters, do we also need to do it for env vars on Unix too? For example, if a parameter has an env var NAME="*.txt", no expansion is done, Click currently expects you to handle that.

I'm not really familiar with how click uses envvars. As a Unix user I would expect NAME=~/foo.txt to work, and be surprised if NAME=*.txt worked. I don't know what people would expect for envvar handling on Windows.

@davidism
Copy link
Member

"Annoying thing that every cli app has to do" seems like it falls within click's scope.

Agree.

I believe in traditional Windows globbing the metacharacters are *, ? only.

It would be less efficient, but the easiest way would be to always pass values through expanduser and glob first.

Is there a way to signal that something glob-like isn't a glob, like single quotes on Unix?

One last thing I thought of. What happens when using invoke (for testing or for dispatching other commands) now? On Windows, invoke(args=["subcommand", "*.txt"]) would expand, but on Unix it wouldn't, which I can see causing inconsistent behavior.

@njsmith
Copy link
Author

njsmith commented Aug 24, 2018

Is there a way to signal that something glob-like isn't a glob, like single quotes on Unix?

I'm not sure. In Windows, quote handling is also done in process (basically you just get a raw command line string and are free to do whatever with it), so in principle you could use quotes to disable globbing for specific arguments. But in Python, the quote handling happens early in interpreter startup, so you can't see them in sys.argv. I guess if you're really masochistic you could call GetCommandLine and try to reparse it, but i can't imagine this working well given all the ways you python can be invoked (e.g. python -m mycli), plus it'd be a ton of fiddly work for unclear benefit.

Python's glob module lets you use [] for quoting. (E.g., the pattern [*].txt matches the literal filename *.txt.) Not very intuitive, but it is possible, and I guess Windows users are used to weird edge cases when trying to use the cli to refer to files with * in the name?

One last thing I thought of. What happens when using invoke (for testing or for dispatching other commands) now? On Windows, invoke(args=["subcommand", "*.txt"]) would expand, but on Unix it wouldn't, which I can see causing inconsistent behavior.

I don't know enough about click and invoke to say anything useful here. (I've spent about an hour with click so far :-).) Tough I guess you could make the argument that the behavior is inconsistent on the two platforms, intentionally so, and your tests should reflect that?

@asweigart
Copy link

asweigart commented Dec 10, 2019

Hello, I came here from the Black project, because running black *.py doesn't work on Windows Command Prompt or PowerShell and I've traced the problem to how Click handles wildcards. Click not handling this results in wildcards simply not working on Windows for every program that uses Click, which seems like a large hole.

How can we safely tell that a name is a glob? There are other patterns besides *.

I think leaning on Python's glob module is suitable enough for handling * and ? (njsmith is correct that [] isn't used on Windows, but we'd get it for free with glob.) And anyways, let's not let perfect be the enemy of good; even just getting some features working would be good. I just want black *.py to work on Windows.

What if the name comes from Bash and *.txt was actually the name of the file after expansion?

To reiterate njsmith, this would have to be a Windows-only feature. The * and ? characters are illegal to have in files on Windows anyway (I assume this is an NTFS thing.)

Do we also want to expand ~?

Nah. This isn't a Windows command line idiom. It'd be easier to leave it unimplemented, given that glob also doesn't implement it.

Anyway, my comments mostly reiterate what njsmith said. I'd just like to shed more light on this issue because "wildcards don't work on Windows" seems like a pretty big deal for every program that uses Click.

@davidism
Copy link
Member

davidism commented Mar 2, 2021

What happens when using invoke (for testing or for dispatching other commands) now? On Windows, invoke(args=["subcommand", "*.txt"]) would expand, but on Unix it wouldn't, which I can see causing inconsistent behavior.

Thinking about this again, processing should only happen in BaseCommand.main, so Context.invoke isn't affected. invoke is already expected to be passed Python values, not unparsed command line values.

So the solution for this is to add a call to glob in BaseCommand.main only on Windows.

@leshnabalara

This comment has been minimized.

@davidism
Copy link
Member

davidism commented Apr 1, 2021

Note that *.txt will only expand if there actually are any .txt files, otherwise Unix shells pass on the literal *.txt and you get the same error as in the op. Not going to do anything about this, since it's standard shell behavior, but it can be unexpected so I wanted to mention it. glob.glob returns an empty list if nothing matches, so I'll replace that with the original pattern in that case to match this behavior.

I think we should also call os.path.expanduser and os.path.expandvars on each argument. If we're expanding globs for better consistency between Windows and Unix, it makes sense to do the other expansions that Unix shells do and Python provides. I'll also enable recursive=True for glob so that ** is expanded.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
windows Issues pertaining to the Windows environment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants