9362 Remove “terrible hack” in t.c.ssh.filetransfer.FileTransferClient

[wiml]

Instead of using the wasAFile dict to store the information needed to wrap a file-handle response in a client-side object, store that information as arguments to the callback which does the wrapping.

This is a fix for https://twistedmatrix.com/trac/ticket/9362 . All of the changed code is covered by existing tests, and there should be no visible change in behavior or exposed api.

[adiroiban]

@wiml thanks for the PR.

Sorry for not observing this PR sooner.... I now see that this is all fine.

My initial comment was about Trac#9373

---

There is a backward compatibility problem with this change.
Even if not documented the FileTransferClient.wasAFile is a public attribute.

Here are more details about the backward compatibility policy. http://twistedmatrix.com/documents/current/core/development/policy/compatibility-policy.html

I don't know any good ready why someone would want to use this public attribute, so I think that this can be merged after following the process for excepting a change from the backward compatibility policy.

here detatils here
http://twistedmatrix.com/documents/current/core/development/policy/compatibility-policy.html#procedure-for-exceptions-to-this-policy

but in short, just send an email with a specific subject to Twisted mailing list, informing the change and the reason for the change.
As long as nobody is against it, and at least one person is for it, it can be merged.

[adiroiban]

even if this is private, it needs docstring.
I know that existing code might have fewer docstring but we should try to raise the bar, at least for the new code.

Is wrapperClass the right name. Wrapper for what?
They look to me like normal objects which are specialized to handle file or directory requests and that they are not wrapping any other object.

I hope that with the docstring, these things will be clear.

[wiml]

In my mind the classes are wrapping the raw filehandle bytes, in order to provide a nice pythonic interface to the filetransfer RPCs. I'd be happy to give that arg a different name if there's something that would be clearer.

[adiroiban]

I don't know what to say. If we go on this line we can say that the any class in conch.ssh is a wrapper, as it just provides a nice interface for the SSH raw bytes... so FileTransferServer is just a wrapper around the SFTP subsystem.

Maybe call it just handlingClass with a docstring along these lines:
'Class to further process the open file or open directory requests.`

In Twisted, a wrapper is usually an object which wraps another object and provides the same interface.

Here we have 2 classes with different API

[wiml]

How about handleClass? That would make a lot of sense to me: it's the class that is given back to user code to act as a file handle, and it also contains the filetransfer-protocol file handle value. Unless that sounds like a bad name to you I'll change it and add a docstring.

(I agree wrapperClass isn't an especially good name, it's just the best name I could think of. It seems like a "wrapper" to me because it mostly passes methods through to the FileTransferClient.)

[adiroiban]

+1 for handleClass

The docstring is needed anyway :)

[adiroiban]

I am curious, why was this getting in your way?
Why do you want this change made, other than just code cleanup?

Thanks!

[wiml]

I am curious, why was this getting in your way?

Mostly just code cleanup. I was reading this code to track down a different bug (https://twistedmatrix.com/trac/ticket/9349) and thought that bit of code was hard to reason about (and confusing: why was it not done the obvious way?). It turned out not to be related to the bug I was fixing after all, but since the original author apparently didn't like the "terrible hack" either, I figured I'd submit a patch. Normally I wouldn't go through this lengthy bug+PR process just for a code quality nit :)

Thanks for the review, I'll respond when I have a few more minutes.

[adiroiban]

I know that the Twisted dev process is hard, but I don't know how else we can enforce the quality.
Any suggestion is much appreciated.

If we start cutting corners, changing public API without any warning, stop documenting the code, we will end with lesser quality and a worse experience for the users of the library.

I can take care of sending the email to the mailing list, but I still need someone else to add docstrings and update the code.

If I will make changes here, then it will need someone else to review this PR.

Thanks!

[wiml]

Ah, it's not the review / code quality focus itself that I find difficult — I agree that's important, and I'm happy to make changes to match your feedback. It's the difficulty of getting a contribution into a form and place that it will be reviewed that's the major hurdle, at least for an outside contributor like me. (Twisted is easier than some projects, at least, since Twisted actually documents the steps I need to take.)

[adiroiban]

It's the difficulty of getting a contribution into a form and place that it will be reviewed that's the major hurdle

I know the pain. Thanks for doing this.

The main problem is that there are not many people doing reviews for Twisted... and those who do are busy with many other projects... and at least for me, GitHub notifications are chaos :)

Please also add an src/twisted/newsfragments/9362.removal file in which the removal of the public attribute is described.

In this way, when the Twisted will be relesed this small detail will be part of the release notes.

Looking forward and an update and I think that once announced this can be merged.

Thanks again!

[codecov]

Codecov Report

Merging #949 into trunk will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            trunk     #949      +/-   ##
==========================================
+ Coverage   91.44%   91.58%   +0.13%     
==========================================
  Files         843      843              
  Lines      149133   149132       -1     
  Branches    13063    13062       -1     
==========================================
+ Hits       136377   136584     +207     
+ Misses      10630    10432     -198     
+ Partials     2126     2116      -10

[adiroiban]

Changes look good. I repharsed the release notes, but feel free to revert of to change that.

Waiting for your confirmation that all the changes that you would like to have are done and that this can be merged.

Thanks!

[wiml]

As far as I'm concerned, this is all good and can be merged. Thanks for shepherding it through the process!

[adiroiban]

I run the tests on buildbot here https://buildbot.twistedmatrix.com/boxes-supported?branch=9362-remove-wasAFile

OSX pass.

Documentation is failing, due to some win32 error, but that is not related to this branch.