Skip to content

Comments

fixtures: some type annotations, remove cyclic dependency#7844

Merged
bluetech merged 2 commits intopytest-dev:masterfrom
bluetech:typing-fixtures
Oct 4, 2020
Merged

fixtures: some type annotations, remove cyclic dependency#7844
bluetech merged 2 commits intopytest-dev:masterfrom
bluetech:typing-fixtures

Conversation

@bluetech
Copy link
Member

@bluetech bluetech commented Oct 3, 2020

Please see the two commits.

@bluetech bluetech changed the title fixtures: some type annotations, remvove cyclic dependency fixtures: some type annotations, remove cyclic dependency Oct 3, 2020
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Looks lovely overall, thanks

)

def applymarker(self, marker) -> None:
def applymarker(self, marker: Union[str, MarkDecorator]) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Should be plain marks as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review.

It seems that plain mark is not accepted here -- this function calls self.node.add_marker(marker) which starts with

        from _pytest.mark import MARK_GEN

        if isinstance(marker, MarkDecorator):
            marker_ = marker
        elif isinstance(marker, str):
            marker_ = getattr(MARK_GEN, marker)
        else:
            raise ValueError("is not a string or pytest.mark.* Marker"

- Change the fixtures plugin to store its one piece of data on the node's
  Store instead of directly.

- Import FixtureLookupError lazily.
@aklajnert
Copy link
Contributor

I always considered using dicts better than a chain of elifs. What's the rationale behind it?

@bluetech
Copy link
Member Author

bluetech commented Oct 4, 2020

Good question, the reasons I changed it are:

  • Due to the cyclic dependency it used to be initialized in a funky way in a hook. (There is still a cycle with _pytest.python so the second commit doesn't solve this.)

  • With the if/elif chain the logic can be localized in one place which IMO is a bit easier to follow.

  • I can use assert_never which means that if a new scope is ever added it will automatically trigger a mypy warning if a case for the new scope isn't added.

  • Another reason I see now is that coverage doesn't work a "dict conditional" but works well with a chain of ifs.

@aklajnert
Copy link
Contributor

I think that the assert_never can still be used with dicts.
Anyway, all the reasons make sense, thanks for explaining.

@nicoddemus
Copy link
Member

Great work!

@bluetech bluetech merged commit 1c08f1d into pytest-dev:master Oct 4, 2020
@bluetech bluetech deleted the typing-fixtures branch October 6, 2020 16:44
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.

4 participants