Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

tsize support #5

Merged
merged 17 commits into from Sep 24, 2012

Conversation

Projects
None yet
2 participants
Contributor

allenap commented Jul 3, 2012

This adds tsize support. In doing so, I had to add an attribute to IReader - size - which returns the size of the file to be read in bytes.

To be honest, I would never make it a property. Why? Because it looks like attribute access, but it actually results in a filesystem hit (for the first time only, though, due to behaviour of t.p.filepath.FilePath.getsize). I would rather have it as a method.

Still, it is not that much of a problem, I think. More of a style thing.

Owner

allenap replied Jul 4, 2012

You're right. I've just hit a problem with having it as a property too: it may need to return a Deferred. I shall change this to be a function.

Owner

shylent commented Jul 4, 2012

I am ready to merge this. I had a little.. thing with size being a property of IReader. I am not sure how you are going to treat my comments on your code, but anyway this particuar issue is of no real importance. If you want size to be a property, so be it - I will merge.

Contributor

allenap commented Jul 4, 2012

Thanks for the review! I'll change the size property to a get_size() function, so that it's more obvious that work might be done (i.e. it can defer).

Contributor

allenap commented Jul 4, 2012

Mmm, actually that might be tricky when it comes to dealing with options. It might be easier to allow get_reader() and get_writer() to defer, but that might also be too lazy. What's your opinion?

Owner

shylent commented Jul 5, 2012

First of all it didn't seem reasonable to me to return a Deferred from get_reader and / or get_writer, because it is not meant to do any actual work. On the other hand, now that I think of it, it actually might do some useful work (actually, it already does that - it opens the file right in the reader's __init__). So there is an excuse for returning a Deferred from get_reader / get_writer after all.

Returning a Deferred from get_size, though, is a different story, since it will require some changes in RemoteOrigin{Read,Write}Session.startProtocol (since processOptions will return a DeferredList).

It seems to me, that the first approach (making get_reader / get_writer) will be a little less messy to implement.

Contributor

allenap commented Jul 5, 2012

get_reader() and get_writer() can now defer. That was a little more work than I at first guessed, but I think it's still fairly clean.

Contributor

allenap commented Jul 11, 2012

If it helps, I've got a pre-canned diff for the last block of changes I've made.

Owner

shylent commented Jul 11, 2012

Okay, I can't really work on it right now, but I will definitely review all
the relevant changes tomorrow.

Sorry for the delay.

On Wed, Jul 11, 2012 at 9:15 PM, Gavin Panella <
reply@reply.github.com

wrote:

If it helps, I've got a pre-canned diff for the last block of
changes I've made.


Reply to this email directly or view it on GitHub:
#5 (comment)

Contributor

allenap commented Jul 12, 2012

Ta! I know you're busy, so don't stress about it. It's now been packaged for Ubuntu and the package carries (or will) a patch to bring trunk forwards with my changes. When you're happy with those changes and they're landed we'll drop that patch.

@shylent shylent and 1 other commented on an outdated diff Jul 12, 2012

tftp/backend.py
@@ -32,7 +33,8 @@ def get_reader(file_name):
@raise BackendError: for any other errors, that were encountered while
attempting to construct a reader
- @return: an object, that provides L{IReader}
+ @return: an object, that provides L{IReader}, or a L{Deferred} that
@shylent

shylent Jul 12, 2012

Owner

To be honest, if I have such a situation (when a method may or may not defer), I assert that it must always defer, so that the interface is simplified. If the implementor does not need or want to return a Deferred, one can always use twisted.internet.defer.succeed. On the plus side, you automatically know that whatever is returned from the method, you can always call addCallback on it.

@allenap

allenap Jul 25, 2012

Contributor

An advantage of making it optional is backward compatibility.

@shylent shylent commented on the diff Jul 12, 2012

tftp/protocol.py
@@ -42,34 +43,39 @@ def datagramReceived(self, datagram, addr):
return self.transport.write(ERRORDatagram.from_code(ERR_ILLEGAL_OP,
"Unknown transfer mode %s, - expected "
"'netascii' or 'octet' (case-insensitive)" % mode).to_wire(), addr)
+
+ self._clock.callLater(0, self._startSession, datagram, addr, mode)
+
+ @inlineCallbacks
@shylent

shylent Jul 12, 2012

Owner

I'd rather not use inlineCallbacks in my code (IMO they complicate debugging and invite ambiguity), but that is purely a matter of taste (especially in this case).

Owner

shylent commented Jul 12, 2012

I've left some comments, one of which might be of some value (or maybe not). Otherwise, looks good.

Contributor

allenap commented Jul 19, 2012

Thanks for all your comments; I agree with them all. I've had to work on some other, related, bits for a while, but I am going to get this done next week.

Contributor

allenap commented Jul 25, 2012

I've removed both inlineCallbacks on TFTP._startSession() and made get_reader() and get_writer() both always return a Deferred. I've added a helper decorator, tftp.utils.deferred, that wraps a function in a call to maybeDeferred.

Contributor

allenap commented Sep 24, 2012

Hi, if there's nothing else outstanding that needs doing, can you pull this now? The Ubuntu package is carrying an 800+ line diff against trunk and I'd like to get that to near zero. There are two more pull requests after this one, one medium sized, and another very small. Thanks, Gavin.

shylent added a commit that referenced this pull request Sep 24, 2012

@shylent shylent merged commit df484eb into shylent:master Sep 24, 2012

Owner

shylent commented Sep 25, 2012

I've merged the pull request. However, I've replaced calls to unittest.TestCase.assertIsNone with assertTrue(foo is None), since they were failing on my python / twisted version (did it in master in my repo). Hope that is not a problem.

Contributor

allenap commented Sep 25, 2012

That's great, thanks for merging!

@allenap allenap deleted the allenap:tsize-support branch Feb 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment