Type hinting on a context manager processing paths

Question

Code (fixed with the help of @juanpa.arrivillaga:

"""Helpers that can be used for context management."""

import logging
import os
from contextlib import contextmanager
from pathlib import Path
from typing import Union, Iterator, Optional

_LOGGER = logging.getLogger(__name__)


@contextmanager
def working_directory(temporary_path: Union[Path, str], initial_path: Optional[Union[Path, str]] = None) -> Iterator[None]:
    """Changes working directory, and returns to `initial_path` on exit. It's needed for PRAW for example,
    because it looks for praw.ini in Path.cwd(), but that file could be kept in a different directory.

    `initial_path` can be used for example to change working directory relative to the script path, or
    to end up in a different directory than Path.cwd() of the calling script.

    Inspiration: https://stackoverflow.com/questions/41742317/how-can-i-change-directory-with-python-pathlib
    """
    _LOGGER.debug('Working directory of the calling script: %s', Path.cwd())
    _LOGGER.debug('temporary_path = %s', temporary_path)
    _LOGGER.debug('initial_path = %s', initial_path)

    if not isinstance(temporary_path, (Path, str)):
        raise TypeError('"temporary_path" is not of type `Path` or `str`')

    if initial_path is None:
        initial_path = Path.cwd()
    else:
        initial_path = Path(initial_path).absolute()
        if not initial_path.is_dir():
            initial_path = initial_path.parent

    try:
        os.chdir(initial_path / temporary_path)
        _LOGGER.debug('Temporarily changed working directory to: %s', Path.cwd())
        yield
    finally:
        os.chdir(initial_path)

Are the typing annotations that I've done correct?

Also, is there a more pythonic way to write this function? What would you change? I'll post it in CodeReview too.

@juanpa.arrivillaga ... but these lines force `initial_path` to be `Path`: `if initial_path is not Path.cwd():`, `initial_path = Path(initial_path).absolute()` — Laurențiu Andronache, Mar 09 '19 at 18:42
no, it absolutely doesn't. What if `if initial_path is not Path.cwd()` is *false* (which by the way, it *will always be false*). Just convert both the `Path` objects at the top of the function. Your `if` doesn't even have an `else`. — juanpa.arrivillaga, Mar 09 '19 at 18:43

juanpa.arrivillaga · Accepted Answer · 2019-07-10T03:10:44.813

The typing is fine, you simply have bugs revealed by mypy in your code. If both initial_path and temporary_path are str then this will fail. That's why it's complaining.

You only convert to Path in one branch you don't have an else (bad practice, IMO), so when you reach the line:

initial_path / temporary_path it could be that both of those are str.

Note, mypy doesn't know this, but if initial_path is not Path.cwd() is not correct. This statement will always be False. Don't compare things with is unless you mean object identity, and in this case, you don't.

Consider:

>>> from pathlib import Path
>>> Path().cwd() is  Path().cwd()
False

Just do:

def working_directory(
    temporary_path: Union[Path, str],
    initial_path: Union[Path, str] = Path.cwd()
) -> Iterator[None]:
    temporary_path = Path(temporary_path)
    initial_path = Path(initial_path)

At the beginning of your function.

According to [this answer](https://stackoverflow.com/questions/49335263/how-to-properly-annotate-a-contextmanager-in-pycharm), the proper type hinting for a context manager in this case is `def working_directory(...) -> ContextManager[Iterator[None]]` where `ContextManager` is imported from the `typing` module — rovyko, Feb 11 '20 at 21:43

Type hinting on a context manager processing paths

1 Answers1