Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Enhancement]: subclassing collections.UserDict over dict #323

Closed
jamesbraza opened this issue Jan 29, 2024 · 3 comments · Fixed by #324
Closed

[Enhancement]: subclassing collections.UserDict over dict #323

jamesbraza opened this issue Jan 29, 2024 · 3 comments · Fixed by #324
Labels
enhancement New feature or request

Comments

@jamesbraza
Copy link

Overview

There is a built-in data structure collections.UserDict that is designed for subclasses of dict.

However, I don't think many people are aware of collections.UserDict, instead I see lots of dict subclasses. I propose a rule to suggest this change.

This request was made as of refurb version 1.28.0.

Proposal

Here is basically what the rule would do:

+++ from collections import UserDict


--- class MyDict(dict):
+++ class MyUserDict(UserDict):
    pass
@jamesbraza jamesbraza added the enhancement New feature or request label Jan 29, 2024
@dosisod
Copy link
Owner

dosisod commented Jan 30, 2024

Thank you @jamesbraza for opening this! I agree, this would be a great check to add.

There might be some sneaky reason why some people extend from dict instead of UserDict, but like you said, I assume most people don't know about UserDict.

@dosisod
Copy link
Owner

dosisod commented Jan 30, 2024

I would also extend this to include UserList and UserString, since it seems people also like to extend from list and str.

dosisod added a commit that referenced this issue Jan 31, 2024
Closes #323.

I am disabling this check by default for 2 reasons:

* `isinstance(x, dict)` fails for `UserDict` objects (including `list`/`str`)
* Changing `dict` to `UserDict` is not a quick fix, and you will probably have
  to change your class definition to get the most out of `UserDict` (ie,
  removing reimplemented methods, something Refurb can't easily detect).

Teams that want to enforce this can enable this option, but overall, it is hard
to deduce what is/is not proper usage of classes subclassing `dict`.
dosisod added a commit that referenced this issue Jan 31, 2024
Closes #323.

I am disabling this check by default for 2 reasons:

* `isinstance(x, dict)` fails for `UserDict` objects (including `list`/`str`)
* Changing `dict` to `UserDict` is not a quick fix, and you will probably have
  to change your class definition to get the most out of `UserDict` (ie,
  removing reimplemented methods, something Refurb can't easily detect).

Teams that want to enforce this can enable this option, but overall, it is hard
to deduce what is/is not proper usage of classes subclassing `dict`.
@jamesbraza
Copy link
Author

Oh wow that was quick turnaround, and thank you @dosisod! I guess the isinstance(some_user_dict, dict) is a decent gotcha, so you made the right call having it be opt-in. Thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants