Opened 5 years ago

Closed 3 years ago

#6494 enhancement closed fixed (fixed)

Docstrings needs to be fixed in twisted.words.protocols.irc

Reported by: Upasana Shukla Owned by: Glyph
Priority: normal Milestone:
Component: words Keywords: easy documentation
Cc: ralphm, Thijs Triemstra, Eeshan Garg, lvh Branch: branches/irc-docstrings-6494-3
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban, glyph

Description

parsemsg: The opening/closing of docstring should be on a line by themselves
IRCClient.ctcpQuery_DCC: The opening/closing of docstring should be on a line by themselves
IRCClient.dccDoSend: The opening/closing of docstring should be on a line by themselves
IRCClient.dccDoResume: The opening/closing of docstring should be on a line by themselves
IRCClient.dccDoAcceptResume: The opening/closing of docstring should be on a line by themselves
IRCClient.ctcpUnknownReply: The opening/closing of docstring should be on a line by themselves
IRCClient.badMessage: The opening/closing of docstring should be on a line by themselves
IRCClient.quirkyMessage: The opening/closing of docstring should be on a line by themselves
IRCClient.handleCommand: The opening/closing of docstring should be on a line by themselves
DccFileReceiveBasic: The opening/closing of docstring should be on a line by themselves
DccFileReceiveBasic.dataReceived: The opening/closing of docstring should be on a line by themselves
DccSendProtocol: The opening/closing of docstring should be on a line by themselves
fileSize: The opening/closing of docstring should be on a line by themselves
DccChat: The opening/closing of docstring should be on a line by themselves
DccChat.__init__: The opening/closing of docstring should be on a line by themselves
dccDescribe: The opening/closing of docstring should be on a line by themselves
DccFileReceive: The opening/closing of docstring should be on a line by themselves
DccFileReceive.set_directory: The opening/closing of docstring should be on a line by themselves
DccFileReceive.set_filename: The opening/closing of docstring should be on a line by themselves
DccFileReceive.set_overwrite: The opening/closing of docstring should be on a line by themselves
DccFileReceive.connectionLost: The opening/closing of docstring should be on a line by themselves

Attachments (5)

bug_6494.patch (7.4 KB) - added by Upasana Shukla 5 years ago.
bug_6494_2.patch (13.7 KB) - added by Eeshan Garg 3 years ago.
bug_6494_3.patch (13.5 KB) - added by Eeshan Garg 3 years ago.
bug_6494_4.patch (13.7 KB) - added by Eeshan Garg 3 years ago.
bug_6494_5.patch (1.7 KB) - added by Eeshan Garg 3 years ago.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: ralphm added

Changed 5 years ago by Upasana Shukla

Attachment: bug_6494.patch added

comment:2 Changed 5 years ago by Upasana Shukla

Cc: Upasana Shukla added
Keywords: easy review added
Owner: Upasana Shukla deleted

comment:3 Changed 5 years ago by Thijs Triemstra

Author: Upasana ShuklaUpasana
Cc: Thijs Triemstra added
Keywords: documentation added; review removed
Owner: set to Upasana Shukla

I believe this ticket is about making the docstrings conform to the Twisted coding standard. It would make sense if you could add documentation for the various functions you touched, eg.:

def parsemsg(s):
    """
    Breaks a message from an IRC server into its prefix, command, and arguments.

    @param s: [description here]
    @type s: [type here, eg. L{str}]
    @rtype: [type here, eg. L{tuple}]
    @return: [description here]
    """

I expect twistedchecker will start complaining about those missing params documentation as soon as we apply this patch, so fixing it directly seems appropriate. If you don't feel up for that task, open a new ticket (with the easy keyword) so you or somebody else can pick up this task later). Thanks.

comment:4 Changed 5 years ago by Upasana Shukla

Cc: Upasana Shukla removed

Alright, I'll add missing docstrings. Thanks for review.

comment:5 Changed 5 years ago by Upasana Shukla

Author: Upasana

comment:6 Changed 3 years ago by Eeshan Garg

Owner: changed from Upasana Shukla to Eeshan Garg
Status: newassigned

Changed 3 years ago by Eeshan Garg

Attachment: bug_6494_2.patch added

comment:7 Changed 3 years ago by Eeshan Garg

Cc: Eeshan Garg added
Keywords: review added
Owner: changed from Eeshan Garg to Glyph
Status: assignednew

Hi!

I added params documentation to all of the functions specified in the ticket description. I am completely new to networking and Twisted and to the world of programming in general. I would love to work on this further if needed because I really want to close this ticket and keep contributing to Twisted. I learned a lot while working on it. It was a lot of fun!

The patch file I would like to get reviewed is 'bug_6494_2.patch'.

I couldn't assign the ticket to the blank entry as it says in the docs(I couldn't find the blank entry anywhere). I asked on #twisted-dev about this and was told that this is due to a trac upgrade and was advised to assign the ticket to glyph for the time being.

Thanks & regards, Eeshan Garg

comment:8 Changed 3 years ago by Glyph

Author: glyph
Branch: branches/irc-docstrings-6494

(In [42995]) Branching to irc-docstrings-6494.

comment:9 Changed 3 years ago by Glyph

Owner: changed from Glyph to Eeshan Garg

Thanks for your contribution, Eeshan.

I've applied the patch to a branch and kicked off the buildbots, and unfortunately twistedchecker isn't quite happy with it - very close though! You can see the new errors here.

Some additional points:

  1. Although this is inconsistent throughout the code because the policy changed at one point, the current style recommendation for referring to built-in types such as str and tuple is L{str} and L{tuple} rather than C{str} and C{tuple}`. This will generate documentation cross-reference links.
  2. Since we are looking forward to a python-3 future (probably in 30 years or so, but still), using str to describe the type of an argument is somewhat ambiguous. We typically use L{unicode} and L{bytes} to be clear about which one we mean, specifically (although unicode isn't actually a defined name in 3.x, it is at least unambiguous). The only time we would use str is to describe the type of things like Python identifiers and the value of __doc__, which are bytes on 2.x and strs on 3.x, in which case we'd say "native str" to be clear that we mean "we are intentionally being ambiguous about the type because it differs based on your python version" rather than just "str" which means "we are being vague about the type because this docstring was written before python 3.x".
  3. As long as you're in here, that "XXX" in the docstring for ctcpUnknownReply should be removed and placed in a comment - or, better yet, a ticket, since it's a to-do and not really information about the code itself. To-dos go in the ticket tracker, not the documentation.
  4. The excValue parameter to badMessage should have a @type as well. Also, the second argument to raise is not "usually" a class instance - for many versions of Python now, it has been required to be an instance of a BaseException subclass. This is what the @type should indicate.
  5. The reason argument to connectionLost is a Failure instance. Why did you think it was a str? Is there some other documentation that needs fixing?

I look forward to re-reviewing this change. Please attach another patch that addresses the above issues when you have time.

comment:10 Changed 3 years ago by Glyph

Keywords: review removed

comment:11 Changed 3 years ago by Eeshan Garg

Hello!

Thanks for reviewing my patch, glyph. It's an honor to have my very first patch to Twisted reviewed by you. I have some specific questions regarding the changes that I need to make. Please allow me to talk about what I understand before I talk about what I am confused about.

I looked at the BuildBot errors, and I will fix the errors in my next patch and I will be more careful and try to avoid my silly mistakes in the future.

I will also need to change all references to built-in types in my patch to use the L{type} syntax.

I will remove the 'XXX' in the docstring for ctcpUnknownReply and file a separate ticket about it.

I will also give the excValue parameter to badMessage a type of L{BaseException}. My mistake here was probably due to my lack of knowledge and I need to learn more and work harder.

I will change the reason argument to connectionLost to have a type of L{Failure}. Me thinking it was an str is also my fault and is downright ignorance on my part. I'll try and avoid such mistakes in the future.

What I am having trouble understanding is how can I determine whether the type of an argument should be a L{bytes} or L{unicode}?. When working on this patch, I read the tests for each function/method I had to write a docstring for and tried to understand what type each argument should have. I would really appreciate it if you could give me some pointer as to how I can determine the type of an argument.

I learned a lot from your advice, sir and I want to keep contributing to Twisted. :)

Thanks & regards, Eeshan Garg

comment:12 Changed 3 years ago by lvh

Cc: lvh added

Well, unfortunately bytes vs unicode is a tricky problem :)

It boils down to this: think about what a particular object *means*. Is it, semantically, some text? If so, unicode; otherwise, bytes.

Of course, this gets tricky fast. Is an URL bytes, or text? (There's, IIUC, one correct answer.) How about an IRI? How about filesystem paths? Those are bytes on Unix but Unicode on Windows, and I'm not even quite sure on OS X (mostly bytes but with some uppercase/lowercase semantics that seem like they're from text-only?)

This is *particularly* bad for old protocols like FTP and IRC, because they hail from simpler times when everything was probably just ASCII but it's okay; pretend it's bytes and shovel it around. So, even though semantically that text message I am sending to you in a PRIVMSG over IRC is clearly *text* (it's English! you can read it!), it may well be bytes, to IRC.

In IRC, the answer is almost always bytes. A quick grep of the patch suggests that you might be done by simply replacing all the L{str}s with L{bytes}.

Changed 3 years ago by Eeshan Garg

Attachment: bug_6494_3.patch added

comment:13 Changed 3 years ago by Eeshan Garg

Hi!

I just submitted a new patch. It's called 'bug_6494_3.patch'.

Thanks to lvh for his guidance and mentorship!

Thanks & regards, Eeshan Garg

comment:14 Changed 3 years ago by Eeshan Garg

Keywords: review added

comment:15 Changed 3 years ago by Eeshan Garg

Owner: changed from Eeshan Garg to Glyph

comment:16 Changed 3 years ago by Eeshan Garg

Hi!

I just filed a ticket regarding the 'XXX' in the docstring for IRCClient.ctcpUnknownReply.

Here is the link to the ticket: https://twistedmatrix.com/trac/ticket/7560

Please let me know if anything else needs to be done. :)

Thanks & regards, Eeshan

comment:17 Changed 3 years ago by Glyph

Owner: Glyph deleted

Un-assigning all review tickets assigned to me. Unless there's a specific reason I need to review something, let's leave them unassigned :-).

comment:18 Changed 3 years ago by Adi Roiban

Owner: set to Adi Roiban

I will try to resolve the conflicts form latest patch and update the branch to see if all is ok...and continue the review

comment:19 Changed 3 years ago by Adi Roiban

Author: glyphadiroiban, glyph
Branch: branches/irc-docstrings-6494branches/irc-docstrings-6494-2

(In [43733]) Branching to irc-docstrings-6494-2.

comment:20 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Eeshan Garg

Thanks for the work on this and sorry for the delay.

I have apply latest patch on a new brand and trigger the tests.

Looks good. Test all green.

Regarding the removal of XXX comment. Not sure if is ok to completly remove it.

-        XXX: If the client makes arbitrary CTCP queries,
-        this method should probably show the responses to
-        them instead of treating them as anomolies.

I prefer to keep a comment like the one below. In this way, when someone stumble upon this problem it is much easier to locate the dedicated ticket and avoid creating a duplicated ticket.

# FIXME:7560:
# Add code for handling arbitrary queries and not treat them as
# anomalies.

But this is minor and I think that we can merge it as it is now.


Regarding

+    @rtype: L{tuple}
+    @return: A 3-tuple of (prefix, command, args) where args is
+        of type L{list}.

Maybe we should put rtype after return and have a simple docstring, as rtype already has a link to L{tuple}.

    @return: A tuple of (prefix, command, args).
    @rtype: L{tuple}

Also this is minor and we can merge it.


If you are ok with the changes you can either submit a new patch or I can apply them and merge it.

Thanks!

Changed 3 years ago by Eeshan Garg

Attachment: bug_6494_4.patch added

comment:21 Changed 3 years ago by Eeshan Garg

Keywords: review added
Owner: changed from Eeshan Garg to Adi Roiban

Hello Mr. adiroiban!

I just added a new patch (bug_6494_4.patch) with all of the changes that you advised me to make. The new patch also includes all of the changes that came with the previous patch, but I resolved all of the conflicts this time. I also included a twisted/words/topfiles/6494.misc file, but whenever I apply the patch to trunk (using patch -p0), the .misc file does not get created due to tool problems.

Please let me know if anything else needs to be done or if something could be better! I would love to work on it! :-)

Warm regards, Eeshan Garg

comment:22 Changed 3 years ago by Adi Roiban

Branch: branches/irc-docstrings-6494-2branches/irc-docstrings-6494-3

(In [43797]) Branching to irc-docstrings-6494-3.

comment:23 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Eeshan Garg

Thanks for the patch. I will handle the misc file ... i can see it in the github diff :)

For https://github.com/twisted/twisted/compare/trunk...irc-docstrings-6494-3#diff-17b06a87bc9a7bc8bbac5bd6df97285eR2625

@ivar bytesReceived: See L{bytesReceived} parameter of L{__init__}

I am not sure how you ended up with this. bytesReceived is not an argument of init. You should document it in the class docstring.


https://github.com/twisted/twisted/compare/trunk...irc-docstrings-6494-3#diff-17b06a87bc9a7bc8bbac5bd6df97285eR2630

def __init__(self, resumeOffset=0):

resumeOffset is not documented


https://github.com/twisted/twisted/compare/trunk...irc-docstrings-6494-3#diff-17b06a87bc9a7bc8bbac5bd6df97285eR2639

def dataReceived(self, data):

data is not documented


https://github.com/twisted/twisted/compare/trunk...irc-docstrings-6494-3#diff-17b06a87bc9a7bc8bbac5bd6df97285eR2342 u A 3-list of [fileName, address, port] }}}

Does it needs to be a list? I think that a tuple should also work.. or any sequence

Now we are documenting existing code..so it might require a list.

Just a comment for the future, when possible we should document the most generic data structure.

For example if a method uses a list just to iterate over its element, the input arguments should be documented as an iterator, rather than a specific list. ho-------

https://github.com/twisted/twisted/compare/trunk...irc-docstrings-6494-3#diff-17b06a87bc9a7bc8bbac5bd6df97285eR2486

@param line: The indecipherable message.

Why this comes without a type?


Not sure why buildbot was happy with the latest patch.

Please address the comments and send a new patch. If possible, please only submit a patch on top of current patch. Let me know if you need help with that.

Thanks!

Changed 3 years ago by Eeshan Garg

Attachment: bug_6494_5.patch added

comment:24 Changed 3 years ago by Eeshan Garg

Keywords: review added
Owner: changed from Eeshan Garg to Adi Roiban

Hello Mr. adiroiban!

I just submitted the new patch (bug_6494_5.patch) that we had a discussion about on IRC. Thanks a lot for your guidance! Please let me know if anything else needs to be done, I would love to work on this further. :-)

Warm regards, Eeshan Garg

comment:25 Changed 3 years ago by Adi Roiban

Owner: Adi Roiban deleted

Thanks for the patch.

For

@param resumeOffset: An integer representing the amount of bytes 
   from where the transfer of data should be resumed. 

reading the code, I can see that this is not what the code is doing. When resumeOffset is not 0, it will just open the file in append mode... this is an naive resume.

If the file has 15bytes, you have already sent 10bytes and then ask a resume at 8bytes following another 7byte, the file will be opened in append resulting a final file of 17bytes

now, maybe this is a bug, but without proper documentation is hard to say if this is a bug or not.


Regarding See: L{protocol.Protocol.dataReceived} we will need to check with the other developers about what is the right format ... but I think that your patch is good.


I am leaving this for review so that another developer can do the final checks.

Thanks!

comment:26 Changed 3 years ago by Adi Roiban

I have also launched a new buildbot run for the latest patch.

comment:27 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Glyph

The builds basically look good, and while I can think of more stuff to document here, this is a big improvement to understanding the existing code, so I will land it.

There are a few places where wrapping / indentation is a little bit off (indentation by a non-multiple of 4 within a docstring, for example) so I will fix that before landing. But this is not even reported by the tools, so I could not expect you to get it right :).

comment:28 Changed 3 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [43873]) Merge irc-docstrings-6494-3: Improve the docstrings in twisted.words.irc.

Author: eeshangarg, Upasana

Reviewer: adiroiban, glyph, thijs

Fixes: #6494

This adds documentation for many undocumented parameters and brings the docstrings much more into line with the coding standard.

Note: See TracTickets for help on using tickets.