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

[UPD] long 2 loops #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johnashu
Copy link

@johnashu johnashu commented Mar 2, 2020

What this PR solves

I was reading through this excellent codebase and just came across 2 situations that were slightly, possibly violating DRY principles.

- It makes it unnecessary in the future to update the reserved_opcodes function if the RESERVED Opcodes become something.

- in TestParser.parser, you could add other items to read/yield easier.

How it is done
Looping instead of repeating code.

What to look out for
I do not have the full test setup on my system so I cannot confirm or deny all Passes / Failures :)

Review

  • Ready for review

yield data
data = yield self.read(2)
yield data
for x in (1, 2, 4, 2):
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally I would approve of such a change, but this code is a throwaway test for debugging, it doesn't run in normal usage. The repetition is somewhat intentional, it makes makes it clear at a glance how many bytes are being read while debugging.

Copy link
Author

Choose a reason for hiding this comment

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

Generally I would approve of such a change, but this code is a throwaway test for debugging, it doesn't run in normal usage. The repetition is somewhat intentional, it makes makes it clear at a glance how many bytes are being read while debugging.

Fair enough :)

Opcode.RESERVED8,
Opcode.RESERVED9,
Opcode.RESERVED10,
Opcode.__dict__[x] for x in Opcode.__dict__.keys() if x.startswith('RESERVED')
Copy link
Contributor

Choose a reason for hiding this comment

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

The original is a bit verbose, I'll grant you. But I don't mind the repetition here. The duplication is in a single file, and a literal has the advantage that it doesn't execute any code at import time. It's also unlikely to change any time soon, as far as I know the websocket protocol has stabilised.

Copy link
Author

Choose a reason for hiding this comment

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

The original is a bit verbose, I'll grant you. But I don't mind the repetition here. The duplication is in a single file, and a literal has the advantage that it doesn't execute any code at import time. It's also unlikely to change any time soon, as far as I know the websocket protocol has stabilised.

Yes, I noticed that the Websockets has not changed since 2011..

I enjoyed reading the code and just could not help but make the change :).. habit

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.

2 participants