Skip to content
This repository was archived by the owner on Sep 14, 2023. It is now read-only.

WIP: file collection after deprecation of 'py' in pytest #114

Closed
wants to merge 1 commit into from
Closed

Conversation

SirUli
Copy link
Contributor

@SirUli SirUli commented Feb 15, 2022

@ssbarnea ssbarnea added the bug This issue/PR relates to a bug. label Feb 15, 2022
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

It seems incomplete. Can you also add type information while you modify it?

@SirUli
Copy link
Contributor Author

SirUli commented Feb 15, 2022

@ssbarnea I hope this fits your expectation

@ssbarnea
Copy link
Member

@ssbarnea I hope this fits your expectation

It is getting close! mypy still fails. Once ready I will merge it and make a release. I don't want to prevent others from using pytest 7.x

@SirUli SirUli changed the title fix: file collection after deprecation of 'py' in pytest WIP: file collection after deprecation of 'py' in pytest Feb 17, 2022

else:

def pytest_collect_file(parent: pytest.Collector, path) -> Optional["Node"]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mypy complains here with:

src/pytest_molecule/__init__.py:142: error: All conditional function variants must have identical signatures

@ssbarnea this is intentional since they actually are different in the pytest versions. Ideas? I tend to ignore the error but maybe you have a better idea for the implementation?

Thanks in advance!

@ssbarnea
Copy link
Member

I would just drop support for older pytest versions and require 7+. I personally do not have the bandwidth to keep compatibility with older versions.

@SirUli
Copy link
Contributor Author

SirUli commented Mar 23, 2022

@ssbarnea i dropped the support for pytest < 7. Sorry for the delay - went on parental leave and forgot about that being open ;)

@ssbarnea ssbarnea mentioned this pull request Mar 25, 2022
@ssbarnea
Copy link
Member

Closing as #121 is implementing it. I tried to update your PR but it was created without permissions for maintainers to update it, so I was forced to create a new one.

Thanks for taking time to do it!

@ssbarnea ssbarnea closed this Mar 25, 2022
@SirUli
Copy link
Contributor Author

SirUli commented Mar 25, 2022

I actually didn't intentionally choose to do so. Perfect - thanks for your help.

Sorry for the lengthy process - i learned quite a bit during it and hope to better contribute to upcoming changes ;)

@ssbarnea
Copy link
Member

@SirUli I hope so. With community projects we never know how long it would take to ship a change. I will be happy to see more PRs from you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants