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

chore(utils): allow duplicate values in registry by making reverse lookup optional #82114

Merged
merged 3 commits into from
Dec 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions src/sentry/utils/registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,15 @@ class NoRegistrationExistsError(ValueError):


class Registry(Generic[T]):
def __init__(self):
"""
A simple generic registry that allows for registering and retrieving items by key. Reverse lookup by value is enabled by default.
If you have duplicate values, you may want to disable reverse lookup.
"""

def __init__(self, enable_reverse_lookup=True):
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth a docstring explaining that it's useful to disable reverse lookup when you have duplicate values

self.registrations: dict[str, T] = {}
self.reverse_lookup: dict[T, str] = {}
self.enable_reverse_lookup = enable_reverse_lookup

def register(self, key: str):
Copy link
Member

Choose a reason for hiding this comment

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

Separately, it could also be worth changing the way we register so that we support both a decorator and a function. Not a huge priority though.

def inner(item: T) -> T:
Expand All @@ -26,13 +32,14 @@ def inner(item: T) -> T:
f"A registration already exists for {key}: {self.registrations[key]}"
)

if item in self.reverse_lookup:
raise AlreadyRegisteredError(
f"A registration already exists for {item}: {self.reverse_lookup[item]}"
)
if self.enable_reverse_lookup:
if item in self.reverse_lookup:
raise AlreadyRegisteredError(
f"A registration already exists for {item}: {self.reverse_lookup[item]}"
)
self.reverse_lookup[item] = key

self.registrations[key] = item
self.reverse_lookup[item] = key

return item

Expand All @@ -44,6 +51,8 @@ def get(self, key: str) -> T:
return self.registrations[key]

def get_key(self, item: T) -> str:
if not self.enable_reverse_lookup:
raise NotImplementedError("Reverse lookup is not enabled")
if item not in self.reverse_lookup:
raise NoRegistrationExistsError(f"No registration exists for {item}")
return self.reverse_lookup[item]
20 changes: 20 additions & 0 deletions tests/sentry/utils/test_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,23 @@ def unregistered_func():

test_registry.register("something else")(unregistered_func)
assert test_registry.get("something else") == unregistered_func

def test_allow_duplicate_values(self):
test_registry = Registry[str](enable_reverse_lookup=False)

@test_registry.register("something")
@test_registry.register("something 2")
def registered_func():
pass

assert test_registry.get("something") == registered_func
cathteng marked this conversation as resolved.
Show resolved Hide resolved
assert test_registry.get("something 2") == registered_func

with pytest.raises(NoRegistrationExistsError):
test_registry.get("something else")

with pytest.raises(NotImplementedError):
test_registry.get_key(registered_func)

test_registry.register("something else")(registered_func)
assert test_registry.get("something else") == registered_func
Loading