search

andrewschenck / paramiko-jump

Enables MFA/2FA and SSH proxying on top of Paramiko SSH.
Other
32 stars 9 forks source link

Typing Update: Typed Arguments/Parameters, Docstring Updates, Version Additions, and README Updates #19

Closed prokopto-dev closed 4 months ago

prokopto-dev commented 4 months ago

Hey @andrewschenck ;

I thought I might write this up and create this MR; I've been using something close to what I've added here for a while, and I think adding the various typing features is useful, especially as newer python modules keep adding these things in.

Features include (as in description):

Only thing I can think of adding here is maybe a version update in the setup.py, but maybe that's not necessary.

andrewschenck commented 4 months ago

It is inappropriate to document fabric here -- fabric, while it may depend on and include this library, is meta to this, and all documentation for fabric should live with fabric, not with the various libraries fabric imports. The principle of single responsibility applies to documentation as well as code, and the documentation in this repo should be solely about how to invoke this code.

I dislike code that documents types in docstrings as well as using typing. Pick one or the other, both are redundant, and essentially violates DRY. If you're going to use type annotations, just worry about documenting behaviors in the docstrings, because every single code analysis tool / IDE is going to pick up the types without the redundant docstrings.

I like using typing in Python where appropriate, but some of the more powerful language features of Python are duck-typing and polymorphism. To that end: Why should I wrap username with a call to str() (perform type coercion) rather than just let username 'quack like a str?' And similar.

I fully intend to drop a py.typed, mypy check this as well as some other python checks, and push this to pypi. I've already got a branch in progress with this work, which I alluded to in email to you previously.

andrewschenck commented 4 months ago

If you want to submit a new PR with a subset of those changes, I’m receptive. Andrew On May 20, 2024, at 10:57, Courtney Caldwell @.***> wrote:Hey @andrewschenck ; I thought I might write this up and create this MR; I've been using something close to what I've added here for a while, and I think adding the various typing features is useful, especially as newer python modules keep adding these things in. Features include (as in description):

README updated to have all codeblocks do format highlighting Added section in README on how to use in fabric (if they don't want to use my package implementation, heh) Added py.typed file to allow the module, when built, to pass through the stubs from the type hints in the descriptions Added types to all definitions in the code, ensuring they match with their pairs in paramiko and work with major typing libraries (checked pyright, mypy, and pylance). Added those types to all existing docstrings Added docstrings where missing, including types

Only thing I can think of adding here is maybe a version update in the setup.py, but maybe that's not necessary.

You can view, comment on, or merge this pull request online at:   https://github.com/andrewschenck/paramiko-jump/pull/19

Commit Summary

3e64e37 refactor: Updated the imports from paramiko to avoid inherited imports 273f0b1 Merge branch 'andrewschenck:master' into master fa66e8f docs: added types, py.typed, and docstrings, as well as README updates

File Changes (4 files)

M
.travis.yml
(2)

M
README.rst
(31)

M
paramiko_jump/client.py
(248)

A
paramiko_jump/py.typed
(0)

Patch Links:

https://github.com/andrewschenck/paramiko-jump/pull/19.patch https://github.com/andrewschenck/paramiko-jump/pull/19.diff

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

prokopto-dev commented 4 months ago

@andrewschenck : Ack, read and received;

Points are lucid and make good technical sense.

will remake with: