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

Utilize more_itertools to consolidate logic when collecting marks. #11156

Closed
wants to merge 1 commit into from

Conversation

jaraco
Copy link
Contributor

@jaraco jaraco commented Jul 1, 2023

No description provided.

@jaraco
Copy link
Contributor Author

jaraco commented Jul 1, 2023

While working on #10447, I noticed two areas for improvement in structures.get_unpacked_marks:

  • The logic to convert "one or many" to a list was repeated in both if isinstance(obj, type) and not.
  • Instead of utilizing a "flatten" operation (e.g. itertools.chain.from_iterable or more_itertools.flatten), the code imperatively constructs a list and selectively appends or extends the list, entangling the flattening operation with the "one or many" conversion.

This approach instead constructs mark_lists in a symmetrical way in each branch and then performs the flattening and "one or many" to list conversion on the result.

I believe this approach is safer because it avoids unnecessary code duplication, reduces mccabe complexity, and makes the code more symmetrical (and thus easier to interpret).

Please consider adopting it.

I did not create a changelog entry because this change is a pure internal refactor with no effect on tests or behavior. I can add a changelog entry linking to this PR if desired. I welcome other feedback as well.

@jaraco
Copy link
Contributor Author

jaraco commented Jul 1, 2023

Note that there are potentially many more places where always_iterable could simplify the code:

diff --git a/src/_pytest/python.py b/src/_pytest/python.py
index 5f4ba3da6..e6318c411 100644
--- a/src/_pytest/python.py
+++ b/src/_pytest/python.py
@@ -29,6 +29,8 @@ from typing import Tuple
 from typing import TYPE_CHECKING
 from typing import Union
 
+from more_itertools import always_iterable
+
 import _pytest
 from _pytest import fixtures
 from _pytest import nodes
@@ -455,12 +457,7 @@ class PyCollector(PyobjMixin, nodes.Collector):
                 res = ihook.pytest_pycollect_makeitem(
                     collector=self, name=name, obj=obj
                 )
-                if res is None:
-                    continue
-                elif isinstance(res, list):
-                    values.extend(res)
-                else:
-                    values.append(res)
+                values.extend(always_iterable(res))
             dict_values.append(values)
 
         # Between classes in the class hierarchy, reverse-MRO order -- nodes

If you'd like me to explore a broader refactor, I can do that on request.

@RonnyPfannschmidt
Copy link
Member

we had it in a while back and it got removed in #7587

@RonnyPfannschmidt
Copy link
Member

personally i'm inclined a bit against it as some of those convenient helpers hide conceptual complexity that sometimes ought to be removed instead of being eased/hidden

for example the support for pytestmark being a single marker or a list is a sloppy convenience and that cost should be visible if not removed

@jaraco
Copy link
Contributor Author

jaraco commented Jul 1, 2023

Fair enough.

@jaraco jaraco closed this Jul 1, 2023
@jaraco jaraco deleted the refactor/more-itertools branch July 1, 2023 22:17
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.

None yet

2 participants