Skip to content

Conversation

bluetech
Copy link
Member

In 92ba96b we have changed the path
attribute to return a pathlib.Path instead of py.path.local without
a deprecation hoping it would be alright. But these types are somewhat
public, reachable through ExceptionInfo.traceback, and broke code in
practice. So restore them in the legacypath plugin and add Path
alternatives under a different name - source_path.

Fix #9423.

@bluetech
Copy link
Member Author

The alternative to this as discussed in the issue is to go through with the breaking change.

For this PR:

  • Honor our promise to not break stuff without deprecation
  • Will restore compat for the promise project and whatever other unknown projects that will be broken by this

Against this PR:

  • source_path is uglier and less discoverable than path
  • We estimate not many users are using this at all
  • Some code might work fine with the silent switch from py.path -> pathlib and will end up better
  • The path fix is temporary as we aim to deprecate and remove py.path stuff eventually

Overall it will be better to go with the breaking change in this case, i.e. close this PR and document the change as breaking rather than trivial as 92ba96b did. WDYT?

@bluetech bluetech added the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label Dec 25, 2021
@bluetech bluetech force-pushed the code-source_path branch 2 times, most recently from 90d3862 to 39b914e Compare December 25, 2021 11:36
In 92ba96b we have changed the `path`
attribute to return a `pathlib.Path` instead of `py.path.local` without
a deprecation hoping it would be alright. But these types are somewhat
public, reachable through `ExceptionInfo.traceback`, and broke code in
practice. So restore them in the legacypath plugin and add `Path`
alternatives under a different name - `source_path`.

Fix pytest-dev#9423.
def cut(
self,
path: Optional[Union[Path, str]] = None,
path: Optional[Union["os.PathLike[str]", str]] = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

This part also fixes the sybil failure - it passes a string absolute path to cut, which used to work because py.path.local('/foo') == '/foo' but Path('/foo') != '/foo'. So the code now compares strings.

Even if we go with the breaking change option we should get this included in pytest 7.0 (this part is non breaking)

@nicoddemus
Copy link
Member

Overall it will be better to go with the breaking change in this case, i.e. close this PR and document the change as breaking rather than trivial as 92ba96b did.

As I commented in the issue, I think we should just go for it, but I'm not against if others prefer to play safe here and go with the PR instead. 👍

@bluetech
Copy link
Member Author

Replaced by #9447.

@bluetech bluetech closed this Dec 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[prerelease] ExceptionInfo.traceback[...].path changed from py.path to pathlib.Path
2 participants