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

click.Path returns str, not pathlib.Path #405

Closed
flying-sheep opened this issue Aug 10, 2015 · 34 comments · Fixed by #1804
Closed

click.Path returns str, not pathlib.Path #405

flying-sheep opened this issue Aug 10, 2015 · 34 comments · Fixed by #1804
Milestone

Comments

@flying-sheep
Copy link
Contributor

It’s surprising that INT returns an int, File a file, STRING a str, but Path a …str?

A click.Path parameter should return a pathlib.Path object.

@untitaker
Copy link
Contributor

pathlib is Python 3 only afaict, Click isn't.

@untitaker
Copy link
Contributor

FWIW I'm certain that an option to return a Path object is useful, but I can't see it becoming the default.

@flying-sheep
Copy link
Contributor Author

it’s backported

@untitaker
Copy link
Contributor

It's still not possible to make this a default since this would mean that the behavior of Click would depend on whether that backport is installed. And depending on that backport is not worth the added value.

@flying-sheep
Copy link
Contributor Author

depending on that backport is not worth the added value.

what cost/value function evaluates to this result? 😄

@untitaker
Copy link
Contributor

In this case, new functionality. Pathlib is merely a new API for the same
thing.

Sorry, but I don't think this is going to happen. I'll gladly accept PRs for a
non-default option though.

On Mon, Aug 10, 2015 at 06:28:08AM -0700, Philipp A. wrote:

depending on that backport is not worth the added value.

what cost/value function evaluates to this result? 😄


Reply to this email directly or view it on GitHub:
#405 (comment)

@flying-sheep
Copy link
Contributor Author

What do you mean with “non-default option”?

@untitaker
Copy link
Contributor

Opt-in for returning a pathlib.Path object instead of a string. The user has to
depend on the pathlib library themselves unless it's in the stdlib. I think
that's doable.

On Mon, Aug 10, 2015 at 06:37:04AM -0700, Philipp A. wrote:

What do you mean with “non-default option”?


Reply to this email directly or view it on GitHub:
#405 (comment)

@flying-sheep
Copy link
Contributor Author

got it, makes sense. best would be to do it on arbitrary levels, right?

e.g. you could create a group that is configured to do it, passing it down to all subcommands and parameters, or on single parameters or even options.

the interaction of this with click.CommandCollection is obvious, but bad: merging a non-pathlib group with a pathlib group would simply behave as if each individual command would have been configured, and result in a mess, but that’s not really a problem i guess, and there’s no alternative anyway

@untitaker
Copy link
Contributor

got it, makes sense. best would be to do it on arbitrary levels, right?

Honestly I don't think that's a good idea, it makes it hard to trace why a particular option is returning this particular type. If you want to avoid repetition/verbosity, you can still bind the created decorator to a new name for reuse, or create a new decorator that has it as default.

@flying-sheep
Copy link
Contributor Author

hmm, yeah, using partial. only commands are defined on the group, so this would be easy.

@mitsuhiko
Copy link
Contributor

Not going to happen. A custom path implementation might be something for addon packages.

@untitaker
Copy link
Contributor

Welp-

@untitaker untitaker reopened this Sep 23, 2015
@flying-sheep
Copy link
Contributor Author

flying-sheep commented Jan 23, 2017

worth reevaluating in the light of PEP 519?

also are you going to sign the python 3 statement? if so, we can tackle this in 2020.

@untitaker
Copy link
Contributor

untitaker commented Jan 23, 2017 via email

@flying-sheep
Copy link
Contributor Author

i didn’t want to propose that, just ask 😉

the PEP partly applies to everything, as it describes a protocol, not just an API.

checking for the existence of __fspath__ works in any version!

@taion
Copy link

taion commented Aug 25, 2017

It would be really confusing to get a pathlib.Path object in 3.6+, but not in 3.5 or lower.

Is gating this on a (non-default) flag on click.Path still out per #405 (comment)?

@mitsuhiko
Copy link
Contributor

Since PATH does not exist anyways we can do two things. One is we set up a default mapping for pathlib.Path as input type (similar to how we have a default type for str) being passed to click.Path(convert=pathlib.Path) and then add support for convert being passed to click.Path.

That way someone can also pass it through something else that does something interesting with the path.

@altendky
Copy link
Contributor

altendky commented Nov 8, 2018

I understand finding time for reviews is hard but I wanted to make sure I didn't need to be doing something else for #405 #1148. I think it fits within the suggested approach of having a generic converter which a developer could pass pathlib.Path to if they so chose.

@altendky
Copy link
Contributor

altendky commented Nov 9, 2018

err, #1148

@NotTheEconomist
Copy link

I'd love to see this. I expected click.Path(path_type=pathlib.PurePath) would cast my result to a PurePath, but @altendky's converter is even more clear. Would love to see this pulled ASAP so I can start using it in my app.

termim added a commit to termim/click that referenced this issue Feb 19, 2019
This would allow to convert path names to pathlib.Path instances
for example.
@termim
Copy link

termim commented Feb 19, 2019

I 'd agree with @NotTheEconomist version without new arguments.
Please consider #1234 .

termim added a commit to termim/click that referenced this issue Feb 19, 2019
This would allow to convert path names to pathlib.Path instances
for example.
termim added a commit to termim/click that referenced this issue Feb 19, 2019
This would allow to convert path names to pathlib.Path instances
for example.
termim added a commit to termim/click that referenced this issue Feb 19, 2019
This would allow to convert path names to pathlib.Path instances
for example.
termim added a commit to termim/click that referenced this issue Feb 21, 2019
This would allow to convert path names to pathlib.Path instances
for example.
termim added a commit to termim/click that referenced this issue Feb 21, 2019
This would allow to convert path names to pathlib.Path instances
for example.
termim added a commit to termim/click that referenced this issue Feb 21, 2019
This would allow to convert path names to pathlib.Path instances
for example.
@jeremyh
Copy link

jeremyh commented Mar 8, 2019

For the impatient, I defined my own like this. It accepts all the same options as click.Path:

class PathPath(click.Path):
    """A Click path argument that returns a pathlib Path, not a string"""
    def convert(self, value, param, ctx):
        return Path(super().convert(value, param, ctx))
@click.argument('input', type=PathPath(dir_okay=False))
def ...

(this usage style also feels cleaner to me than click.Path(...., converter=pathlib.Path), as well as more easily remembered. Path is built into Python's standard library and widely used, so I think it's worth a dedicated type.)

@adamtheturtle
Copy link
Contributor

Thanks @jeremyh !

I have packaged this solution into https://pypi.org/project/click-pathlib, so you can use:

$ pip install click-pathlib
import click
import click_pathlib

@click.command('delete')
@click.argument(
    'existing_file',
    type=click_pathlib.Path(exists=True),
)
def delete(existing_file):
    existing_file.unlink()

@jrderuiter
Copy link

Seeing as Python 2 is now dead and all, does it maybe make sense to now transition to using pathlib.Path as a default instead of a str value?

@ThiefMaster
Copy link
Member

Pretty sure that would require a major release, and while pathlib is awesome, I think it's the kind of change that would be a pain in the ass for any downstream user who doesn't already use pathlib.

@expobrain
Copy link

Also, even if Path is available on Python 3.x it's not mandatory to use it, a developer can still choose to use plain string or a Path instance

@untitaker
Copy link
Contributor

Any request for making this the default basically asks for two things:

  • New major version of click, break all existing code
  • Drop Python 2 or pull in dependency on backport

Each of those points in isolation is an extreme measure to make this happen, combined even more.

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Jan 16, 2020

The second one isn’t, Python 2 isn’t supported anymore so you’re free to move on and get rid of all hideous compatibility hacks 🎉

About the first … click has a lot of major version releases, and derive from your reluctance that they’re all less intrusive than this one?

@untitaker
Copy link
Contributor

Click supports Python 2.

@flying-sheep
Copy link
Contributor Author

You know I mean that Python 2 is unmaintained as of 2 weeks ago. Supporting something that’s unmaintained is pretty futile.

@untitaker
Copy link
Contributor

I'm saying that this is a completely separate decision to make. But I forgot that we already announced dropped support for Python 2 so this would probably be fine to land in Click 8.

https://palletsproject.com/blog/ending-python2-support/

@flying-sheep
Copy link
Contributor Author

flying-sheep commented Jan 16, 2020

Hmm, just spitballing here, but maybe a transitional release could be done (after #1148 is merged). In Click 8, the following would raise a FutureWarning like this:

@click.argument('filename', type=click.Path())

You specified no Path converter. click.Path(converter=None) will result in a pathlib.Path object in Click 9. To avoid this warning, specify converter=str or converter=pathlib.Path.

And consequently, in Click 9 (or 10), the warning would be gone and the default changed.

@flying-sheep
Copy link
Contributor Author

An alternative (also depending on #1148) would be to follow @mitsuhiko’s idea in #405 (comment):

We just add click.PATH and use it in examples instead of click.Path, therefore encouraging click.PATH without breaking changes. This would be less disruptive.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet