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

Trivial code cleanup #448

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Trivial code cleanup #448

merged 2 commits into from
Nov 15, 2024

Conversation

kgaughan
Copy link
Member

This is mostly stuff picked up by ruff check --fix with some extra fixes to the type checking.

A few assertion clean-ups leaked across from #447, but I don't think that's too big a deal.

This is mostly stuff picked up by `ruff check --fix` with some extra
fixes to the type checking.
@@ -349,7 +349,7 @@ def write_soon(self, data):
raise ClientDisconnected
num_bytes = len(data)

if data.__class__ is ReadOnlyFileBasedBuffer:
if isinstance(data, ReadOnlyFileBasedBuffer):
Copy link
Member

Choose a reason for hiding this comment

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

There was a reason this was a data.__class__ check instead of isinstance, and something broke in the past when I tried to update this. I just can't remember what exactly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't able to find anything in the history. That line goes back well over a decade, and Chris didn't leave any explanation of why it uses .__class__ is ... rather than isinstance(...). I can revert the code to use the former and instead leave a "there be dragons" comment here and in task.py, if that works?

Copy link
Member

Choose a reason for hiding this comment

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

It was probably me avoiding a funccall for performance reasons while I was doing profiling.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a big fan of "if tests pass..." so let's let it ride for now.

I will re-review this tomorrow when I am not sleep deprived, and hope that I can remember what the issue was.

@digitalresistor digitalresistor merged commit d005ec2 into Pylons:main Nov 15, 2024
38 checks passed
@kgaughan kgaughan deleted the trivial-cleanup branch November 15, 2024 09:33
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.

3 participants