allow passing kwargs to sftp client

[ttomasz]

Fixes #39

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.64%. Comparing base (15aac74) to head (032c69e).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
- Coverage   94.30%   93.64%   -0.67%     
==========================================
  Files          13       12       -1     
  Lines         738      803      +65     
==========================================
+ Hits          696      752      +56     
- Misses         42       51       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ttomasz]

@efiop Would you mind reviewing this PR?

[efiop]

Hey, @ttomasz , thanks for the PR! Please see some suggestions above.

[shcheklein]

@ttomasz a gentle reminder :) or can we close this PR?

[ttomasz]

Sorry, was quite busy past few months 😅 I should be able to finish this up in June.

[ttomasz]

Changed popping kwargs to separate parameter for stuff to pass to underlying library.

[ttomasz]

Sorry for the mess with the commits. Do you want me to revert client_args/connect_args to being kwargs instead of parameter? Or is the current state ok?

[erikvanzijst]

Can I ask for this PR to get revived? I too need to pass options to asyncssh.SSHClientConnection.start_sftp_client().

I need to override the default sftp_version due to hitting ronf/asyncssh#587

[Copilot]

🟡 Not ready to approve

As written it likely doesn’t fully resolve Issue #39 because path_encoding passed as a top-level kwarg would still be forwarded to asyncssh.connect(), and the new behavior also needs targeted tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR adds a way to pass asyncssh.SSHClientConnection.start_sftp_client() keyword arguments through SSHFileSystem, enabling configuration such as path_encoding (referenced in Issue #39) without overloading asyncssh.connect() options.

Changes:

• Add sftp_client_kwargs to SSHFileSystem and plumb it through the connection/pool setup.
• Update the SFTP channel pool base class to store sftp_client_kwargs and forward them into start_sftp_client(**...).
• Refactor connection argument passing to distinguish connect_args from SFTP-client args.

File summaries

File	Description
sshfs/spec.py	Adds sftp_client_kwargs and separates connect_args from SFTP-client kwargs during connection/pool creation.
sshfs/pools/base.py	Stores sftp_client_kwargs on the pool and forwards them to client.start_sftp_client(**...).

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[shcheklein]

Rebased this onto main (squashed the branch history into a single commit, keeping your authorship) and pushed:

• d7190a2 — your change, rebased
• d02015d — a test covering sftp_client_kwargs end to end, plus minor doc polish

To answer the open question: the explicit sftp_client_kwargs / connect_args parameters are the right shape — no need to revert to kwargs-popping. Thanks for the patience on this one, and thanks @erikvanzijst for the nudge.

[Copilot]

🟡 Not ready to approve

It currently introduces a backward-incompatible API behavior by always passing sftp_client_kwargs into pool_type(...), which will break existing custom pool implementations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

sshfs/spec.py:126

• sftp_client_kwargs is always normalized to {} and then unconditionally passed into pool_type(...). That makes every custom pool_type (which previously only needed to accept client and max_channels) start failing with TypeError: __init__() got an unexpected keyword argument 'sftp_client_kwargs' even when the caller doesn’t use this new feature. Consider only passing sftp_client_kwargs to the pool when it’s actually provided/non-empty, so existing custom pool implementations remain compatible by default.

        pool = pool_type(
            client,
            max_channels=max_sftp_channels,
            sftp_client_kwargs=sftp_client_kwargs,
        )

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[shcheklein]

Re Copilot's "not ready to approve" note about always passing sftp_client_kwargs into pool_type(...): intentional, keeping as-is. pool_type is an sshfs-specific extension point — custom pools are expected to subclass BaseSFTPChannelPool, whose __init__ already accepts the option (plus **kwargs), so the natural implementation keeps working; both shipped pools do. Passing the option only when non-empty would just trade an obvious construction-time TypeError for a delayed one that fires the first time someone actually uses sftp_client_kwargs with a legacy pool. Not part of the common fsspec public API surface, so not treated as a compatibility blocker.