Use dataclasses more liberally throughout the codebase #12571

ichard26 · 2024-03-13T01:43:45Z

This seemed like an easy refactoring. Please let me know if any/all of these conversions should be dropped.

Convert numerous internal classes to dataclasses for readability and stricter enforcement of immutability across the codebase. A conservative approach was taken in selecting which classes to convert. Classes which did not convert cleanly into a dataclass or were "too complex" (e.g. maintains interconnected state) were left alone.

Unfortunately due to dataclasses' poor handling of slots under 3.10, converting the class to a dataclass is not an option. I would get these errors which would be nontrivial to work around. ValueError: '_hashes' in __slots__ conflicts with class variable However, this is the last use of KeyBasedCompareMixin so after this commit, it can be removed entirely!

I left ArchiveInfo alone as it has some non-trivial attribute logic that I would rather not touch.

And drop a test that's now raising type errors as HiddenText can no longer be compared with strings.

ichard26 · 2024-03-13T01:46:04Z

src/pip/_internal/models/direct_url.py

+@dataclass
 class VcsInfo:


I opted to leave the direct_url classes non-frozen as some tests broke and it would be more consistent with ArchiveInfo which I did not convert (due to non-trivial logic I didn't feel like groking).

ichard26 · 2024-03-13T01:47:35Z

src/pip/_internal/vcs/git.py

@@ -217,7 +218,7 @@ def resolve_revision(

        if sha is not None:
            rev_options = rev_options.make_new(sha)


RevOptions.make_new() seems like unnecessary boilerplate, but I've opted to leave additional (potential) refactoring to another PR.

ichard26 · 2024-03-13T01:48:42Z

src/pip/_internal/index/package_finder.py

+@dataclass
 class CandidatePreferences:


The codebase does assign to these attributes from time to time...

pradyunsg · 2024-04-07T23:06:31Z

src/pip/_internal/models/scheme.py


 SCHEME_KEYS = ["platlib", "purelib", "headers", "scripts", "data"]


+@dataclass


Why can we not freeze this class?

pip/src/pip/_internal/locations/_sysconfig.py

Lines 194 to 197 in 31437d6

if root is not None:

for key in SCHEME_KEYS:

value = change_root(root, getattr(scheme, key))

setattr(scheme, key, value)

Although this is trivial to fix... so I did that in 3738d25. Hopefully it doesn't break the test suite.

src/pip/_internal/index/collector.py

pradyunsg · 2024-04-08T12:24:13Z

tests/unit/test_utils.py

@@ -802,20 +802,6 @@ def test_basic(self) -> None:
        assert hidden.redacted == "######"
        assert hidden.secret == "my-secret"

-    def test_equality_with_str(self) -> None:


Hmm... I think this test should have been preserved.

The comparisons in this test are no longer type-safe which is why I removed it. I can adjust the class to support comparisons with strings if that's wanted.

mypy.....................................................................Failed - hook id: mypy - exit code: 1 tests/unit/test_utils.py:813: error: Non-overlapping equality check (left operand type: "HiddenText", right operand type: "str") [comparison-overlap] assert hidden != hidden.secret ^~~~~~~~~~~~~~~~~~~~~~~ tests/unit/test_utils.py:814: error: Non-overlapping equality check (left operand type: "str", right operand type: "HiddenText") [comparison-overlap] assert hidden.secret != hidden ^~~~~~~~~~~~~~~~~~~~~~~ tests/unit/test_utils.py:816: error: Non-overlapping equality check (left operand type: "HiddenText", right operand type: "str") [comparison-overlap] assert hidden != hidden.redacted ^~~~~~~~~~~~~~~~~~~~~~~~~ tests/unit/test_utils.py:817: error: Non-overlapping equality check (left operand type: "str", right operand type: "HiddenText") [comparison-overlap] assert hidden.redacted != hidden ^~~~~~~~~~~~~~~~~~~~~~~~~ Found 4 errors in 1 file (checked 292 source files)

That's because you removed the __eq__ comparison from the HiddenText class... (Edit: More accurately, because the equality implementation added by the dataclass code doesn't have the same type signature as the one it replaced).

It doesn't seem like the class was meant to be compared with strings in the first place (other than in tests) so it's debatable whether it's worth keeping it, but I've rolled this back in bde667d anyway.

pip/src/pip/_internal/utils/misc.py

Lines 594 to 601 in 31437d6

# This is useful for testing.

def __eq__(self, other: Any) -> bool:

if type(self) != type(other):

return False

# The string being used for redaction doesn't also have to match,

# just the raw, original string.

return self.secret == other.secret

Edit: I've talked to some people more knowledgeable in typing than me and IIUC these type errors shouldn't be raised in the first place... Sometimes, I hate typing ¯\_(ツ)_/¯

ichard26 · 2024-04-17T22:59:38Z

Conflicts have been resolved.

ichard26 added 14 commits March 12, 2024 17:37

Convert Scheme into a dataclass

c7c4043

Convert InstallationCandidate into a dataclass

a643731

Remove KeyBasedCompareMixin

a70b593

Convert IndexContent and CandidatePreferences to dataclasses

42c1b69

Convert most of models.direct_url to dataclasses

534dab3

I left ArchiveInfo alone as it has some non-trivial attribute logic that I would rather not touch.

Convert SearchScope to a dataclass

924e0f0

Add TODO for SelectionPreferences

d3596d0

Convert prepare.File to a dataclass

7cd8b0e

Convert RequirementParts and InstallationResult to dataclasses

4f317b2

Convert Constraint to a dataclass

39970e6

Convert HiddenText to a dataclass

29a452c

And drop a test that's now raising type errors as HiddenText can no longer be compared with strings.

Convert RevOptions into a dataclass

7f67070

📰

39af6fe

psf-chronographer bot added the bot:chronographer:provided label Mar 13, 2024

ichard26 commented Mar 13, 2024

View reviewed changes

pradyunsg approved these changes Apr 7, 2024

View reviewed changes

Freeze locations' Scheme dataclass

3738d25

uranusjr reviewed Apr 8, 2024

View reviewed changes

src/pip/_internal/index/collector.py Outdated Show resolved Hide resolved

pradyunsg reviewed Apr 8, 2024

View reviewed changes

ichard26 added 2 commits April 8, 2024 10:20

Trim unnecessary lines

b8cf8fa

Restore HiddenText.__eq__ and associated test

bde667d

pradyunsg approved these changes Apr 8, 2024

View reviewed changes

Merge branch 'main' into cleanup-dataclasses

d8afb77

pradyunsg merged commit 4d09e3c into pypa:main Apr 18, 2024
24 checks passed

sbidoul mentioned this pull request Apr 21, 2024

Cache is_satisfied_by #12453

Merged

hugovk mentioned this pull request May 1, 2024

[TODO][CI][pip upstream changes] Fix failing nightlies running against pip's main branch jazzband/pip-tools#2078

Closed

ichard26 deleted the cleanup-dataclasses branch May 1, 2024 23:00

github-actions bot locked as resolved and limited conversation to collaborators May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dataclasses more liberally throughout the codebase #12571

Use dataclasses more liberally throughout the codebase #12571

ichard26 commented Mar 13, 2024

ichard26 Mar 13, 2024

ichard26 Mar 13, 2024

ichard26 Mar 13, 2024

pradyunsg Apr 7, 2024

pradyunsg Apr 7, 2024

ichard26 Apr 8, 2024

pradyunsg Apr 8, 2024

ichard26 Apr 8, 2024

pfmoore Apr 8, 2024 •

edited

Loading

ichard26 Apr 8, 2024 •

edited

Loading

ichard26 commented Apr 17, 2024

		@@ -217,7 +218,7 @@ def resolve_revision(

		if sha is not None:
		rev_options = rev_options.make_new(sha)


		SCHEME_KEYS = ["platlib", "purelib", "headers", "scripts", "data"]


		@dataclass

	if root is not None:
	for key in SCHEME_KEYS:
	value = change_root(root, getattr(scheme, key))
	setattr(scheme, key, value)

	# This is useful for testing.
	def __eq__(self, other: Any) -> bool:
	if type(self) != type(other):
	return False

	# The string being used for redaction doesn't also have to match,
	# just the raw, original string.
	return self.secret == other.secret

Use dataclasses more liberally throughout the codebase #12571

Use dataclasses more liberally throughout the codebase #12571

Conversation

ichard26 commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfmoore Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

ichard26 Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

ichard26 commented Apr 17, 2024

pfmoore Apr 8, 2024 •

edited

Loading

ichard26 Apr 8, 2024 •

edited

Loading