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

fix handling of env-vars passed to plumbum Commands; support new with_cwd #513

Merged
merged 2 commits into from Jan 26, 2021

Conversation

koreno
Copy link
Collaborator

@koreno koreno commented Jun 16, 2020

Fix handling of env-vars passed to plumbum BoundEnvCommands:
- don't use the non-threadsafe and session-dependent .env() context manager
- sync popen support for 'env' param in all machine impls: local/ssh/paramiko
- add .with_cwd() to complement .with_env()

@coveralls
Copy link

coveralls commented Jun 16, 2020

Coverage Status

Coverage increased (+0.0002%) to 82.293% when pulling e4db361 on fix-cmd-with-env into f817e62 on master.

@AndydeCleyre
Copy link
Contributor

Is there more documentation for with_cwd? How does it relate to the usual local.cwd? Would the following be valid?

from plumbum import local
from plumbum.cmd import pwd

with local.cwd('/tmp'):
    pwd.with_cwd('/home')().strip() == '/home'
    pwd().strip() == '/tmp'

@henryiii henryiii self-assigned this Sep 10, 2020
@henryiii henryiii closed this Jan 3, 2021
@henryiii henryiii reopened this Jan 3, 2021
@henryiii
Copy link
Collaborator

henryiii commented Jan 3, 2021

Could you please address @AndydeCleyre's question? I'm a little worried if we are just adding another way to spell something that already exists, that's just more to maintain and adapt for Plumbum 2.0.

@koreno
Copy link
Collaborator Author

koreno commented Jan 11, 2021

Is there more documentation for with_cwd? How does it relate to the usual local.cwd? Would the following be valid?

from plumbum import local
from plumbum.cmd import pwd

with local.cwd('/tmp'):
    pwd.with_cwd('/home')().strip() == '/home'
    pwd().strip() == '/tmp'

sorry it took so long -
I would expect the with_cwd to override the ctxmgr, since it is set directly on the command object. Only command objects that don't have their own cwd will fall back on the default, which is modified by the ctxmgr.
I will add a test for this, and docs.

What do you think?

@henryiii
Copy link
Collaborator

With test and docs, I'd be fine with it.

@koreno
Copy link
Collaborator Author

koreno commented Jan 17, 2021

@henryiii - ci is failing on coverall actions, not sure how to proceed...

@henryiii henryiii closed this Jan 17, 2021
@henryiii henryiii reopened this Jan 17, 2021
@henryiii
Copy link
Collaborator

henryiii commented Jan 17, 2021

Looks like it is due to this: coverallsapp/github-action#7

We'll probably have to disable coverage on PRs that are forked, unless there's some other way around this. We can print a report, but we can't upload it to coveralls. pull_request_target is a new option, but it runs on the base and not the merged state, so can't be used for coverage.

@henryiii
Copy link
Collaborator

Oh, that's odd, this is not a fork. Should be getting a read/write token. (Probably still need to fix this for forks, but not as sure this is the problem).

@henryiii
Copy link
Collaborator

Coveralls 3 broke GitHub service detection ( TheKevJames/coveralls-python#252 ), so I now manually specify it and it should be working again.

… new with_cwd

- don't use the non-threadsafe and session-dependent .env() context manager
- sync popen support for 'env' param in all machine impls: local/ssh/paramiko
- add .with_cwd() to complement .with_env()
@henryiii henryiii merged commit e1bc3f1 into master Jan 26, 2021
@henryiii henryiii deleted the fix-cmd-with-env branch January 26, 2021 04:27
@henryiii henryiii added this to the v1.7.0 milestone Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants