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

Make Session.chdir() a context manager #543

Merged

Conversation

Tolker-KU
Copy link
Contributor

Closes #512. Also see the discussion here: #513.

I'm a bit unsure if the changes to the docs are sufficient. Please suggest changes as you see fit.

@Tolker-KU Tolker-KU force-pushed the feature/chdir_as_context_manager branch 2 times, most recently from 87c0386 to 2b1424d Compare December 27, 2021 18:44
@Tolker-KU Tolker-KU force-pushed the feature/chdir_as_context_manager branch from 2b1424d to 3ae10ac Compare December 27, 2021 18:47
nox/sessions.py Outdated Show resolved Hide resolved
@FollowTheProcess
Copy link
Collaborator

Could this be simplified further to a functional context manager? i.e. you wouldn't need the WorkingDirContext class at all?

Something like:

@contextlib.contextmanager
def chdir(self, to: str) -> Iterator:
    original = os.getcwd()
    try:
        os.chdir(to)
        yield
    finally:
        os.chdir(original)

I haven't done much with context managers tbh so I'm not 100% that these are equivalent

@Tolker-KU
Copy link
Contributor Author

Could this be simplified further to a functional context manager? i.e. you wouldn't need the WorkingDirContext class at all?

Something like:

@contextlib.contextmanager
def chdir(self, to: str) -> Iterator:
    original = os.getcwd()
    try:
        os.chdir(to)
        yield
    finally:
        os.chdir(original)

I haven't done much with context managers tbh so I'm not 100% that these are equivalent

As far as I know the problem with this is that it would break the current functionality.

@nox.session()
def some_session(session):
    session.chdir("path/to/somewhere")
    # Clean has been run here, so we are back in old working dir

Using the class approach we are able to keep the current functionality at the cost of some complexity.

@FollowTheProcess
Copy link
Collaborator

Could this be simplified further to a functional context manager? i.e. you wouldn't need the WorkingDirContext class at all?

Something like:

@contextlib.contextmanager

def chdir(self, to: str) -> Iterator:

original = os.getcwd()
try:
    os.chdir(to)
    yield
finally:
    os.chdir(original)

I haven't done much with context managers tbh so I'm not 100% that these are equivalent

As far as I know the problem with this is that it would break the current functionality.

@nox.session()

def some_session(session):

    session.chdir("path/to/somewhere")

    # Clean has been run here, so we are back in old working dir

Using the class approach we are able to keep the current functionality at the cost of some complexity.

Cool, makes sense, thanks! This looks good to me then

@FollowTheProcess FollowTheProcess merged commit f55e9c5 into wntrblm:main Dec 28, 2021
@Tolker-KU Tolker-KU deleted the feature/chdir_as_context_manager branch December 28, 2021 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Run commands from within a specific directory
4 participants