Skip to content

Conversation

@whyvineet
Copy link

@whyvineet whyvineet commented Jan 25, 2026

Simplify single-field dataclass ordering comparisons
#144191

@whyvineet whyvineet requested a review from ericvsmith as a code owner January 25, 2026 15:33
@bedevere-app
Copy link

bedevere-app bot commented Jan 25, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Copy link
Member

@johnslavik johnslavik left a comment

Choose a reason for hiding this comment

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

Thanks! You can wait for the green light from Eric before going forward or add tests for this even now. I'd probably change test_1_field_compare. And of course a news entry, too.

Comment on lines 1183 to 1188
if len(flds) == 1:
self_expr = f"self.{flds[0].name}"
other_expr = f"other.{flds[0].name}"
else:
self_expr = _tuple_str('self', flds)
other_expr = _tuple_str('other', flds)
Copy link
Member

@johnslavik johnslavik Jan 26, 2026

Choose a reason for hiding this comment

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

This is how I'd approach it:

Suggested change
if len(flds) == 1:
self_expr = f"self.{flds[0].name}"
other_expr = f"other.{flds[0].name}"
else:
self_expr = _tuple_str('self', flds)
other_expr = _tuple_str('other', flds)
match flds:
# Special-case single field comparisons. See GH-144191.
case [single_fld]:
self_expr = f'self.{single_fld.name}'
other_expr = f'other.{single_fld.name}'
case _:
self_expr = _tuple_str('self', flds)
other_expr = _tuple_str('other', flds)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I’ve updated the logic using match/case. It’s more readable and makes the intent clearer.

Copy link
Member

Choose a reason for hiding this comment

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

Match constructions are known to be way slower than bare ifs. Considering that people sometimes complain that dataclasses are slow, and could impact import times of large modules, can we have a little benchmark introduced by this change?

Copy link
Member

Choose a reason for hiding this comment

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

If this would slow down the function significantly, I take this back.
I haven't considered performance impact.
I'll see if I can put together a benchmark, but at earliest on the weekend.

Copy link
Member

Choose a reason for hiding this comment

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

A similarly idiomatic version with bare ifs:

if len(flds) == 1:
    (single_fld,) = flds
    self_expr = f"self.{single_fld.name}"
    other_expr = f"other.{single_fld.name}"
else:
    self_expr = _tuple_str('self', flds)
    other_expr = _tuple_str('other', flds)

But at this point it's just cosmetics. Feel free to ignore.

@bedevere-app
Copy link

bedevere-app bot commented Jan 26, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants