Skip to content

Implement importlib.abc.InspectLoader in AssertionRewritingHook #7885

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ronaldoussoren
Copy link

This is a first take at a PR for issue #7804, at this time without tests and documentation updates.

At this time I'm primarily interested to hear if the code changes are acceptable.

closes #7804.


Here is a quick checklist that should be present in PRs.

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

If this change fixes an issue, please:

  • Add text like closes #XYZW to the PR description and/or commits (where XYZW is the issue number). See the github docs for more information.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

    Write sentences in the past or present tense, examples:

    • Improved verbose diff output with sequences.
    • Terminal summary statistics now use multiple colors.

    Also make sure to end the sentence with a ..

  • Add yourself to AUTHORS in alphabetical order.

@bluetech
Copy link
Member

This would also fix #6643. In there, @iwanb suggested a different approach of subclassing importlib.abc.FileLoader and importlib.abc.SourceLoader. I didn't check what it actually means, but can you comment on that?

@ronaldoussoren
Copy link
Author

IMHO it is easier to just implement is_package. I'm not entirely happy with my implementation, it would be better to just cache the value (similarly to the _fullname_to_path attribute). I'll push an update to the PR later.

@ronaldoussoren
Copy link
Author

I'm pretty sure that subclassing FileLoader and SourceLoader would require splitting the loader functionality from the rewriting hook. That wouldn't be too hard, but wouldn't buy much and requires more code.

@ronaldoussoren
Copy link
Author

Something else: I'm not sure what's the best way to add tests for the new methods. I'm not a pytest user myself.

Returns the code for a rewritten test file.
"""
# The type annotation is *Optional[...]* to match the declaration
# in importlib.abc.InspectLoader
Copy link
Member

Choose a reason for hiding this comment

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

this comment is unnecessary -- the type checker will enforce this for us (remove the other one as well)

try:
fn = Path(self._fullname_to_info[fullname][0])
except KeyError:
raise ImportError(fullname)
Copy link
Member

Choose a reason for hiding this comment

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

is this error possible? this shouldn't be needed in a PEP451 loader (the code retrieval step doesn't happen if the finder isn't returned) -- this makes me think that we've reverted back to 302

Copy link
Member

Choose a reason for hiding this comment

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

yeah InspectLoader is the 302 interface which would regress us back into imp land

Base automatically changed from master to main March 9, 2021 20:40
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.

Implement importlib.abc.InspectLoader in AssertionRewritingHook
3 participants