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

chore(internal): better support for Python legacy versions in ModuleWatchdog #4083

Merged
merged 7 commits into from
Aug 16, 2022

Conversation

P403n1x87
Copy link
Contributor

@P403n1x87 P403n1x87 commented Aug 13, 2022

Description

This change improves support for Python legacy versions, like 2.7 and 3.5 by using a common loader wrapper that makes more attributes available upon request, e.g. is_package. Furthermore, this change also provides support for dict copy via the dict constructor, which is required by some pytest fixtures, like testdir.

More technical details

In Python<=3.5, calling the dict constructor on a dictionary makes a copy of it by first checking if it has a keys attribute, and then performing a PyDict_Check. If these checks succeed, the dictionary is copied using the C API. Since ModuleWatchdog is a subclass of dict, this means that the wrapping logic is bypassed, and dict ends up copying the dictionary backing ModuleWatchdog rather than the wrapped sys.modules. We exploit the keys attribute access to then copy the state of sys.modules over to the backing dict, so that calling dict on a ModuleWatchdog instance actually creates a copy of the wrapped dictionary instead of the backing one.

Testing

This change is a pre-requisite for #4070 to make the test suite pass.

Checklist

Reviewer Checklist

  • Title is accurate.
  • Description motivates each change.
  • No unnecessary changes were introduced in this PR.
  • PR cannot be broken up into smaller PRs.
  • Avoid breaking API changes unless absolutely necessary.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Release note has been added for fixes and features, or else changelog/no-changelog label added.
  • All relevant GitHub issues are correctly linked.
  • Backports are identified and tagged with Mergifyio.
  • Add to milestone.

…atchdog

This change improves support for Python legacy versions, like 2.7 and
3.5 by using a common loader wrapper that makes more attributes
available upon request, e.g. is_package. Furthermore, this change also
provides support for dict copy via the dict constructor, which is
required by some pytest fixtures, like testdir.
@P403n1x87 P403n1x87 added the changelog/no-changelog A changelog entry is not required for this PR. label Aug 13, 2022
@P403n1x87 P403n1x87 requested a review from a team as a code owner August 13, 2022 09:57
@P403n1x87 P403n1x87 force-pushed the chore/module-better-legacy-support branch from 7683b21 to 9c5a5f8 Compare August 15, 2022 16:09
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

are there any tests we can add to validate the copying is working as expected?

…atchdog

This change improves support for Python legacy versions, like 2.7 and
3.5 by using a common loader wrapper that makes more attributes
available upon request, e.g. is_package. Furthermore, this change also
provides support for dict copy via the dict constructor, which is
required by some pytest fixtures, like testdir.
@P403n1x87 P403n1x87 force-pushed the chore/module-better-legacy-support branch from 9c5a5f8 to cfdcf67 Compare August 16, 2022 12:55
@P403n1x87
Copy link
Contributor Author

are there any tests we can add to validate the copying is working as expected?

I've added a test case.

@P403n1x87 P403n1x87 force-pushed the chore/module-better-legacy-support branch from 764c664 to 0bf6160 Compare August 16, 2022 14:03
@mergify mergify bot merged commit 2f3d954 into DataDog:1.x Aug 16, 2022
@P403n1x87 P403n1x87 deleted the chore/module-better-legacy-support branch August 17, 2022 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants