Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add docstrings to fields of urllib.parse results #70008

Closed
serhiy-storchaka opened this issue Dec 8, 2015 · 10 comments
Closed

Add docstrings to fields of urllib.parse results #70008

serhiy-storchaka opened this issue Dec 8, 2015 · 10 comments
Assignees
@orsenthil
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

BPO 25822
Nosy @rhettinger, @orsenthil, @berkerpeksag, @vadmium, @serhiy-storchaka
Files
  • iss_25822.patch: Fix for named tuple docstring.
  • iss_25822_2.patch: Modified from previous review.
  • iss_25822_3.patch
  • iss_25822_4.patch: Added docstring for DefragResult.
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/orsenthil'
closed_at = <Date 2016-01-14.08:17:37.561>
created_at = <Date 2015-12-08.09:15:40.334>
labels = ['type-feature', 'library']
title = 'Add docstrings to fields of urllib.parse results'
updated_at = <Date 2016-01-15.05:29:35.891>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2016-01-15.05:29:35.891>
actor = 'curioswati'
assignee = 'orsenthil'
closed = True
closed_date = <Date 2016-01-14.08:17:37.561>
closer = 'orsenthil'
components = ['Library (Lib)']
creation = <Date 2015-12-08.09:15:40.334>
creator = 'serhiy.storchaka'
dependencies = []
files = ['41283', '41290', '41300', '41456']
hgrepos = []
issue_num = 25822
keywords = ['patch']
message_count = 10.0
messages = ['256105', '256186', '256197', '256241', '256259', '256364', '257237', '258186', '258187', '258267']
nosy_count = 7.0
nosy_names = ['rhettinger', 'orsenthil', 'python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'curioswati']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue25822'
versions = ['Python 3.6']
    @serhiy-storchaka
    Copy link
    Member Author

    Results of urlsplit() and urlparse() functions in the urllib.parse module are named tuple with a number of fields. Since property docstrings are writable now (bpo-24064), we can provide docstrings for all these fields. See also bpo-24878.

    @serhiy-storchaka serhiy-storchaka added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Dec 8, 2015
    @curioswati
    Copy link
    Mannequin

    curioswati mannequin commented Dec 11, 2015

    I can help with this. Should I propose the changes I am going to make before making them or just create the patch?

    @curioswati
    Copy link
    Mannequin

    curioswati mannequin commented Dec 11, 2015

    Here is the patch, please review it. Do I need to write any test?

    @berkerpeksag
    Copy link
    Member

    Thanks, Swati. I left a few comments on Rietveld: http://bugs.python.org/review/25822/

    A test wouldn't hurt, but you can wait for further review comments to avoid updating tests each time you get a comment.

    @vadmium
    Copy link
    Member

    vadmium commented Dec 12, 2015

    [padding]
    I left some comments.

    I wonder if the doc strings are too specific when they mention “request”, “file”, “download”, “page”, etc. Maybe these could just be examples of what the fields are used for (e.g. “The hierarchical path, such as the path to a file to download”). Or maybe they could be changed to general terms, but this may be hard.

    Also, some of them seem redundant. Does it really add anything to say the “query” field is “the query parameter”?

    @rhettinger
    Copy link
    Contributor

    I wouldn't normally write a test for a docstring patch.

    @serhiy-storchaka
    Copy link
    Member Author

    Could you please also add docstrings for _DefragResultBase?

    I agree that there is no need to write tests.

    @orsenthil orsenthil self-assigned this Jan 14, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 14, 2016

    New changeset 7bfcb8b75ad9 by Senthil Kumaran in branch 'default':
    Issue bpo-25822: Add docstrings to the fields of urllib.parse results.
    https://hg.python.org/cpython/rev/7bfcb8b75ad9

    @orsenthil
    Copy link
    Member

    Thank you, Swati.

    I reviewed and committed your patch. I had to make some minor formatting changes while keeping an eye for the maintainability of the module and doc strings.

    • Moved all the doc strings up to a single place just below the namespaces are declared.
    • Followed pep-0257 conventions for doc strings.
    • Tried DRY for parse result's sub-components.

    Thanks for addressing the review comments from other developers.

    @curioswati
    Copy link
    Mannequin

    curioswati mannequin commented Jan 15, 2016

    Thank you Senthil for the improvements, I'll try to make it better next time :)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @orsenthil orsenthil

    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @berkerpeksag @orsenthil @vadmium @rhettinger @serhiy-storchaka