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

Fully typed RangeMap and avoid complete iterations to find matches #16

Merged
merged 8 commits into from
Aug 25, 2024

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented Jul 27, 2024

Bringing improvements over from mhammond/pywin32#2334

@Avasam Avasam force-pushed the Fully-type-RangeMap branch from ee53c39 to 6ae6e4e Compare July 27, 2024 04:27
Copy link
Owner

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

Looks good. I appreciate the contrib, especially the more efficient use of the filter.

I'd prefer to see contributions like these applied in separate commits for each concern (i.e. 3 commits for the described changes plus additional commits for any style changes). No need to re-write as different commits now, but please consider it for the future. Probably the changes to win32timezone should have been contributed here first and then vendored downstream, but that opportunity has passed.

@@ -119,7 +126,7 @@ def dict_map(function, dictionary):
return dict((key, function(value)) for key, value in dictionary.items())


class RangeMap(dict):
class RangeMap(Dict[int, _VT]):
Copy link
Owner

Choose a reason for hiding this comment

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

Oh. I didn't know you could subclass from Dict. TIL.

int is overspecified here. RangeMap accepts any keys (possibly heterogeneous) that are comparable based on the key_match_comparator.

Also, the values don't need to be uniform. They can be heterogeneous.

Copy link
Contributor Author

@Avasam Avasam Jul 27, 2024

Choose a reason for hiding this comment

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

Oh. I didn't know you could subclass from Dict. TIL.

Yeah typing re-exports runtime generic aliases of builtin and collection types. It's useful when trying to specify generic parameters for builtins and collections that are not yet subscriptable in older Python versions.

Also, the values don't need to be uniform. They can be heterogeneous.

Is RangeMap meant to be mutable? If not I think it would make sense to make the value covariant like a Mapping

Copy link
Owner

Choose a reason for hiding this comment

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

Is RangeMap meant to be mutable?

I was not intending for it to be restricted to be immutable. I can't recall if I've used it for an application that changes it dynamically, but it's a use-case that should be supported unless there's a practical benefit to making it immutable. It was modeled after the dict, so is meant to have similar flexibility.

Copy link
Contributor Author

@Avasam Avasam Jul 27, 2024

Choose a reason for hiding this comment

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

Alright, in that case I think it's correct to keep both keys and values invariant.

jaraco/collections/__init__.py Outdated Show resolved Hide resolved
jaraco/collections/__init__.py Outdated Show resolved Hide resolved
@Avasam
Copy link
Contributor Author

Avasam commented Jul 27, 2024

I'd prefer to see contributions like these applied in separate commits for each concern (i.e. 3 commits for the described changes plus additional commits for any style changes). No need to re-write as different commits now, but please consider it for the future.

I can rebase into clean commits once you've fully approved the changes if you'd like. (unless it was for review purposes, and not a clean commit history, in which case please squash merge).

Probably the changes to win32timezone should have been contributed here first and then vendored downstream

It was kinda done at the same time (midway through changes I figured I should probably provide the improvements back upstream), at which point I may as well complete and push my changes to not loose or forget it. But chances are this PR will be completed and included in a new version before mhammond reviews the downstream PR, and even then I'll just wait before merging.

@jaraco
Copy link
Owner

jaraco commented Jul 27, 2024

I can rebase into clean commits once you've fully approved the changes if you'd like. (unless it was for review purposes, and not a clean commit history, in which case please squash merge).

I'll plan to do a squash merge. I tend to avoid them, but happy to employ in select cases like this one.

@Avasam Avasam requested a review from jaraco July 27, 2024 21:53
@Avasam
Copy link
Contributor Author

Avasam commented Aug 18, 2024

@jaraco is there anything I can do about these docs warnings causing failure ?

@jaraco
Copy link
Owner

jaraco commented Aug 25, 2024

I let Gemini do the hard work, applied in 228c7ba.

@jaraco jaraco merged commit ed2bc06 into jaraco:main Aug 25, 2024
15 checks passed
@Avasam Avasam deleted the Fully-type-RangeMap branch August 25, 2024 17:53
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.

2 participants