Accept pathlib.Path object in Context.cd

[jimm-domingo]

Allow the path parameter to be a pathlib.Path instance as well as a string parameter. As a convenience as pathlib.Path are used more and more for path handling. Thanks.

[bitprophet]

On one hand, I'm not sure how we could go about this without actually depending on pathlib existing. On the other, I do use pathlib or similar libs more often lately myself, and wouldn't be super opposed to using it internally where sensible, which would then open us up to vendoring it & thus enabling this feature.

[brutus]

I just stumbled upon this while investigating a somewhat misleading error message:

$ inv isort
checking import statement order in '/home/brutus/Projekte/LinterTest/src'…
Traceback (most recent call last):
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/config.py", line 97, in __getattr__
    return self._get(key)
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/config.py", line 164, in _get
    value = self._config[key]
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/config.py", line 153, in __getitem__
    return self._get(key)
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/config.py", line 164, in _get
    value = self._config[key]
KeyError: 'cwd'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/brutus/Projekte/LinterTest/.venv/bin/inv", line 11, in <module>
    load_entry_point('invoke==0.20.2', 'console_scripts', 'inv')()
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/program.py", line 294, in run
    self.execute()
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/program.py", line 409, in execute
    executor.execute(*self.tasks)
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/executor.py", line 129, in execute
    result = call.task(*args, **call.kwargs)
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/tasks.py", line 109, in __call__
    result = self.body(*args, **kwargs)
  File "/home/brutus/Projekte/LinterTest/tasks/isort.py", line 31, in check
    ctx.run(cmd)
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/context.py", line 82, in run
    return self._run(runner, command, **kwargs)
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/context.py", line 88, in _run
    command = self._prefix_commands(command)
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/context.py", line 223, in _prefix_commands
    current_directory = self.cwd
  File "/home/brutus/Projekte/LinterTest/.venv/lib/python3.6/site-packages/invoke/config.py", line 109, in __getattr__
    raise AttributeError(err)
AttributeError: No attribute or config key found for 'cwd'

Valid keys: ['isort', 'path', 'run', 'runners', 'sudo', 'tasks']

Valid real attributes: ['cd', 'clear', 'config', 'cwd', 'from_data', 'pop', 'popitem', 'prefix', 'run', 'setdefault', 'sudo', 'update']

I used a Path object instead of a string as argument for ctx.cd. If I cast it to a a string, it works.

So… how about just casting the argument to a string at the start of the method? No need to depend on pathlib for it. Just duck typing ;)

e.g. master...brutus:patch-1

[fndari]

I was also recently bitten by this (I don't remember for sure, but I think not for the first time...), so I prepared a very basic pull request implementing @brutus's suggestion: #583.

[m1keil]

I had the exact same problem. I wonder if there is anything stopping the PR from being merged?

[nateph]

wondering about this as well. would be a great feature to have.

[JnyJny]

The pull request #583 seems to address this problem, any reason to not accept it?

[vxx1]

Path objects are really neat, would be great to use them in invoke's tasks

[mariocesar]

My PR is fixing the same issue -> #681

[rspeed]

This project still supports Python 2.7, which doesn't include pathlib. Though considering that Python 2.7 is itself now unsupported, perhaps now would be a good time to consider dropping support.

Edit: Whoops, I should have looked at the PRs first. Yeah, no need to actually add it as a dependency since casting to str does the job.

[miso-belica]

I think it's OK to just convert the path to string for Invoke because it runs the commands on the compatible OS but I am not sure if it doesn't break Fabric. Fabric uses the path on remote server and doing str(path) on Windows produces .\paths\with\backslashes. When I use Path on windows with Fabric I always use path.as_posix() so remote machine running Linux understands the path separator. To do that you need to know the object. But it think it's still doable just with more care for Fabric compatibility probably 🙂

[rspeed]

Unless I'm mistaken, you could simply use pathlib.PurePosixPath.

[beanaroo]

Looking forward to this functionality! We use Pathlib where possible as it provides great cross-platform operations. It looks like the PR #583 has been accepted and just has some conflicts to resolve.

[topiaruss]

I just hit this snag. It would be nice to see it progress.

[bitprophet]

@miso-belica Arguably, as you and @rspeed touch on, that situation requires forethought on the part of the user. At the very least it would need to be covered under fabric/fabric#1752.

For the purposes of what Invoke can or should do at its level, in the usual "work well for Invoke-only users, and try not to actively get in the way of client libraries like Fabric" approach, I think the solution in the linked PRs (and #607) is probably fine: expect that the given values can be safely cast to strings if they aren't already. Backwards compatible, works with Path objects of most stripes, works with other stuff too.

Insofar as it's not perfect for Fabric, it's still an improvement for anyone using eg PosixPath (since right now those users are manually calling str()) and is no worse otherwise. So Fabric can take its time figuring out that abovelinked issue.

I'll be merging #607 shortly, after tweaking the test suite to more explicitly test that contract instead of Path objects specifically.