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

9713 conch types #12142

Merged
merged 11 commits into from May 1, 2024
Merged

9713 conch types #12142

merged 11 commits into from May 1, 2024

Conversation

glyph
Copy link
Member

@glyph glyph commented Apr 24, 2024

Scope and purpose

Fixes #12141
Fixes #9713

This fixes an interactive traceback with Conch, but also adds a bunch of type annotations and testing problems to ensure that other related issues are easier to fix.

@adiroiban
Copy link
Member

Thanks Glyph for the PR. I will be traveling in the next day... but I will try to get this review.
If somehow I forgot to review this by the start of next week, feel free to ping me and I will take a look.

Thanks again.

@glyph
Copy link
Member Author

glyph commented Apr 25, 2024

Thanks Adi! I just had a fun time accidentally causing my machine to kernel panic with a test bug :) but this should be reviewable now...

@glyph
Copy link
Member Author

glyph commented Apr 25, 2024

please review

@chevah-robot chevah-robot requested a review from a team April 25, 2024 03:40
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

Maybe I got the changes wrong.

I am not sure the changes from bytes IO to str IO are ok.

There is no misisng code coverage ... and I was expecting the test to fail.

Please see the inline comments related to mode="wb" vs mode="b"

Other than that, this looks good.

Thanks again.


@return: an IKnownHostEntry representing the hostname and key in the
input line.
input line.

@rtype: L{PlainEntry}
Copy link
Member

Choose a reason for hiding this comment

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

Minor comment. Maybe this docstring can be removed as pydoctor should be able to extract this from the method signature.

I see the rtype was removed in other methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will get rid of it.

src/twisted/conch/client/knownhosts.py Show resolved Hide resolved
@@ -598,20 +601,19 @@ def body(ignored):
answer = f.readline().strip().lower()
if answer == b"yes":
return True
elif answer == b"no":
elif answer in {b"no", b""}:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe update the docstring

-'yes' and L{False} when the user answers 'no'. It may errback if
-there were any I/O errors.
+'yes' and L{False} when the user answers 'no' or no answer.
+It may errback if there were any I/O errors.

src/twisted/conch/endpoints.py Show resolved Hide resolved
Discard writes because we are emulating a console object, where they'd
go to the screen, not back into the buffer to be read like an a+ file.
"""
return 0
Copy link
Member

Choose a reason for hiding this comment

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

can we assert that we write str or bytes ?

I thins that ByteIO will reject str for a write operation, but it looks _WriteDiscarder will accept anything.

If this is on purpose, I think that this needs to mentioned in the docstring of _WriteDiscarder

src/twisted/conch/test/test_endpoints.py Show resolved Hide resolved
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

All good. Thanks for the explanations. Much appreciated.

@glyph
Copy link
Member Author

glyph commented Apr 29, 2024

All good. Thanks for the explanations. Much appreciated.

Quick approval appreciated as well!

@glyph glyph merged commit ac0d3a5 into trunk May 1, 2024
24 checks passed
@glyph glyph deleted the 9713-conch-types branch May 1, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants