-
Notifications
You must be signed in to change notification settings - Fork 9
Migrate from attrs to dataclasses #124
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
Conversation
else: | ||
kwargs = bind_arguments(self.processor, args, kwargs) | ||
return self.processor(**kwargs) | ||
return self.processor(**kwargs) # type:ignore[call-arg] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add this ignore even before making any changes. Running mypy failed with:
sqs_workers/async_task.py:65: error: Too few arguments [call-arg]
sqs_workers/async_task.py:68: error: Too few arguments [call-arg]
Found 2 errors in 1 file (checked 25 source files)
I did spent a couple of minutes trying to grasp the problem, but I couldn't. I'm not familiar with this codebase, so it may be more obvious to you. But if not, we can keep this for the time being. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same on line 65, it's the same problem)
d3478dc
to
7d82091
Compare
7d82091
to
89983bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Changes looks sane. There's not good way to test them other than updating in the parent project and testing there, which I did not do.
We have reasonable test coverage. :) |
Thanks! |
Migrates from attrs to dataclasses. Claude did most of the heavy lifting, with my supervision. :)
Beyond tests (
memory
andlocalstack
), I leveraged ruff and mypy liberally for this.