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

Confusing docstring on dns.Set #1063

Closed
Zaczero opened this issue Mar 4, 2024 · 4 comments
Closed

Confusing docstring on dns.Set #1063

Zaczero opened this issue Mar 4, 2024 · 4 comments

Comments

@Zaczero
Copy link

Zaczero commented Mar 4, 2024

Describe the bug
The docstring says "these sets are based on lists and are thus indexable".

Specifically,

  1. The Set is based on dicts and not lists.
  2. The __getitem__ method is very inefficient, whenever scalar index is requested, it iterates over half of the dictionary on average. The current docstring suggests that such indexing operation would be fairly inexpensive.

My personal recommendation would be to use Python standard set and not to reinvent the wheel. The CPython sets are very well optimized for both CPU and memory usage. It will also reduce code maintenance and package installation size 🙂.

Context:

  • dnspython version 2.6.1
  • Python version 3.12.2
@Zaczero
Copy link
Author

Zaczero commented Mar 4, 2024

I can see that some classes override the Set methods, perhaps we could do:

class Test(set):  # <-- inherits from set
    def add(self, __element) -> None:
        super().add(5)
        return super().add(__element)


x = Test()
x.add(3)
print(x)
# Test({3, 5})

@rthalley
Copy link
Owner

rthalley commented Mar 4, 2024

We can fix the docstring, but we are stuck with non-native sets for backwards compatibility reasons until dnspython 3.0 at least. As the comment says, the API was designed before there even were sets in Python, and there is lots of code in the wild assuming you can subscript an RRset, especially with [0].

rthalley added a commit that referenced this issue Mar 4, 2024
@rthalley
Copy link
Owner

rthalley commented Mar 4, 2024

Documentation improved.

@rthalley rthalley closed this as completed Mar 4, 2024
@Zaczero
Copy link
Author

Zaczero commented Mar 4, 2024

Alright! Thank you for your time and understanding. Ping me whenever you start accepting donations.

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

No branches or pull requests

2 participants