Skip to content

Honor the copy parameter _get_adjusted_env() #7399

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

Merged
merged 7 commits into from
Aug 10, 2023

Conversation

christian-monch
Copy link
Contributor

This PR modifies WitlessRunner._get_adjusted_env() to honor the copy-parameter and perform a copy only if copy is True.
In addition, it improves the docstring of the method.

This commit lets WitlessRunner._get_adjusted_env()
honor the copy parameter.

It also improves the docstring of the method.
@christian-monch christian-monch added the semver-performance Changes only improve performance, no impact on version label Jun 2, 2023
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +2.74 🎉

Comparison is base (13aaeeb) 88.76% compared to head (ae2f408) 91.51%.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7399      +/-   ##
==========================================
+ Coverage   88.76%   91.51%   +2.74%     
==========================================
  Files         327      325       -2     
  Lines       44693    43408    -1285     
  Branches     5924     5825      -99     
==========================================
+ Hits        39671    39724      +53     
+ Misses       5007     3668    -1339     
- Partials       15       16       +1     
Impacted Files Coverage Δ
datalad/runner/runner.py 100.00% <100.00%> (ø)
datalad/runner/tests/test_witless_runner.py 97.07% <100.00%> (+0.22%) ⬆️

... and 53 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments, but since the change is quite profound but also easy to test -- would be great to see a dedicated unittest (if we had it to start with - unlikely we grew such a bug)

christian-monch and others added 2 commits June 26, 2023 09:16
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
This commit adds a unit test for
Runner._get_adjusted_env to ensure the
correct copy-behavior, i.e. copy an env
only if `copy` is `True`.
@christian-monch
Copy link
Contributor Author

Hi @yarikoptic , thanks for the review. Yes, a test is a good ideat. I added it. Thanks for the hint.

This commit ensures locally, i.e. independent
of previous control flow, that containment is
tested against dictionaries and not against
None-values.

The type checker does not determine that a
non-None original environment will lead to
a non-None adjusted environment, when
Runner._get_adjusted_env is called. We help
it with an `assert` that states this fact.
@yarikoptic yarikoptic merged commit d09636c into datalad:maint Aug 10, 2023
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.19.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-performance Changes only improve performance, no impact on version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants