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

feat: update actor_name assignment in decorator #641

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

z0z0r4
Copy link
Contributor

@z0z0r4 z0z0r4 commented Jul 16, 2024

Close #640

@Bogdanp
Copy link
Owner

Bogdanp commented Jul 16, 2024

This isn't the right approach, since it would be a breaking change to any currently-deployed code. I would prefer it if we checked for duplicate actor names and raised an error instead.

@z0z0r4
Copy link
Contributor Author

z0z0r4 commented Jul 16, 2024

Indeed, I think checking for duplicate actor_name is necessary. But at the same time, it is reasonable to change the default value. We should support functions decorated with the same function name by default, right? This should also be a design mistake.

@Bogdanp
Copy link
Owner

Bogdanp commented Jul 16, 2024

I think it's more nuanced than a plain design mistake. At least, I remember thinking about this quite a bit when I made the original decision to use __name__. For example, even if we used __qualname__ here, renaming the module in which an actor is defined would then potentially lead to a production bug (actors are looked up by name, and messages already enqueued for mod1.foo would not be able to find mod2.foo). Using __qualname__ now is out of the question since it would break users' code for the same reason. So, the only good option is to keep using __name__ and to error when there are duplicates.

@z0z0r4
Copy link
Contributor Author

z0z0r4 commented Jul 16, 2024

actors are looked up by name, and messages already enqueued for mod1.foo would not be able to find mod2.foo

Will anyone who in the production environment keep the queue unemptied and update dramatiq? I don't think so, but there is no evidence.

I will keep this PR until I open another PR to add the exception you mentioned. Delay for a few days .

Revert "Update actor.py"

This reverts commit 3e8ac4a.

feat: raise duplicated actoe_name
@z0z0r4
Copy link
Contributor Author

z0z0r4 commented Jul 27, 2024

@Bogdanp fixed

dramatiq/actor.py Outdated Show resolved Hide resolved
@z0z0r4
Copy link
Contributor Author

z0z0r4 commented Aug 6, 2024

fixed

@Bogdanp Bogdanp merged commit 931bc02 into Bogdanp:master Aug 6, 2024
0 of 11 checks passed
@Bogdanp
Copy link
Owner

Bogdanp commented Aug 6, 2024

Thanks!

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.

Recognize actor with module name instead of only func name
2 participants