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.

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

make wsgiref.headers.Headers accept empty constructor #50050

Closed
tarekziade mannequin opened this issue Apr 20, 2009 · 26 comments
Closed

make wsgiref.headers.Headers accept empty constructor #50050

tarekziade mannequin opened this issue Apr 20, 2009 · 26 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@tarekziade
Copy link
Mannequin

tarekziade mannequin commented Apr 20, 2009

BPO 5800
Nosy @pjeby, @tarekziade, @merwok, @bitdancer, @berkerpeksag
Files
  • patch
  • issue5800.diff
  • issue5800.diff
  • issue5800_v2.diff
  • issue5800_v3.diff
  • 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/berkerpeksag'
    closed_at = <Date 2014-07-02.05:38:54.044>
    created_at = <Date 2009-04-20.16:01:04.297>
    labels = ['easy', 'type-feature', 'library']
    title = 'make wsgiref.headers.Headers accept empty constructor'
    updated_at = <Date 2014-07-02.05:38:54.043>
    user = 'https://github.com/tarekziade'

    bugs.python.org fields:

    activity = <Date 2014-07-02.05:38:54.043>
    actor = 'berker.peksag'
    assignee = 'berker.peksag'
    closed = True
    closed_date = <Date 2014-07-02.05:38:54.044>
    closer = 'berker.peksag'
    components = ['Library (Lib)']
    creation = <Date 2009-04-20.16:01:04.297>
    creator = 'tarek'
    dependencies = []
    files = ['14311', '21016', '35721', '35787', '35814']
    hgrepos = []
    issue_num = 5800
    keywords = ['patch', 'easy', 'needs review']
    message_count = 26.0
    messages = ['86199', '89458', '89459', '89484', '121680', '121681', '121690', '121829', '121958', '121959', '122276', '122318', '122323', '122352', '130169', '130181', '130752', '131578', '221243', '221247', '221248', '221650', '222020', '222033', '222069', '222070']
    nosy_count = 10.0
    nosy_names = ['pje', 'tarek', 'eric.araujo', 'r.david.murray', 'ptn', 'SilentGhost', 'BreamoreBoy', 'ramiroluz', 'python-dev', 'berker.peksag']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue5800'
    versions = ['Python 3.5']

    @tarekziade
    Copy link
    Mannequin Author

    tarekziade mannequin commented Apr 20, 2009

    wsgiref.headers.Headers will let you manage a collection of HTTP
    response headers, but the constructor forces you to provide a list:

    >>> from wsgiref.headers import Headers
    >>> headers = Headers([])
    >>> headers.add_header('Content-Type', 'text/plain')

    A logical change would be to allowed creating an empty Headers instance:

    >>> from wsgiref.headers import Headers
    >>> headers = Headers()
    >>> headers.add_header('Content-Type', 'text/plain')

    @tarekziade tarekziade mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Apr 20, 2009
    @devdanzin devdanzin mannequin added the easy label Apr 22, 2009
    @ptn
    Copy link
    Mannequin

    ptn mannequin commented Jun 17, 2009

    Patch attached. While I was at it, I also removed stupid whitespace and
    generally made the module more PEP-8-compliant.

    @tarekziade
    Copy link
    Mannequin Author

    tarekziade mannequin commented Jun 17, 2009

    I have added another issue for PEP-8 compliancy at bpo-5801

    @ptn
    Copy link
    Mannequin

    ptn mannequin commented Jun 17, 2009

    Added a pointer from bpo-5801 to here.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Nov 20, 2010

    Correct and update patch + update test case

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Nov 20, 2010

    here is the updated test case

    @ramiroluz
    Copy link
    Mannequin

    ramiroluz mannequin commented Nov 20, 2010

    I applied the patches for wsgiref.headers and test_headers.py, ran the test with runtests.sh test_headers.py and it passed.

    I also tried the code in the description:

    >> from wsgiref.headers import Headers
    >> headers = Headers()
    >> headers.add_header('Content-Type', 'text/plain')
    >>

    @merwok
    Copy link
    Member

    merwok commented Nov 21, 2010

    FTR, note that “svn diff file1 file2...” will give you one file for many edits. It’s easier to review and apply.

    Regarding the change, I don’t know if wsgiref 3.2 has to be compatible with Python 2.1, which would exclude using the ternary operation.

    The change from type to isinstance will probably be rejected, since WSGI does not accept subclasses for the things it defines.

    @pjeby
    Copy link
    Mannequin

    pjeby mannequin commented Nov 21, 2010

    Yes, please consider the type->isinstance part of the change rejected. I just got done reverting a bunch of those in 3.2. Where WSGI specifies types, it means "type() is", not "isinstance".

    (The 3.x version of wsgiref does not need to be executable on 2.x versions, but this doesn't mean the protocol itself can be altered in backwards-incompatible ways, just the implementation.)

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Nov 21, 2010

    Do I have to resubmit the patch or can you use the existing one?

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Nov 24, 2010

    Re-submitting the patch for Lib/wsgiref/headers.py w/o the isinstance change

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Nov 24, 2010

    Here is the correction for the docs. I would love to see this making it into 3.2 release.

    @ramiroluz
    Copy link
    Mannequin

    ramiroluz mannequin commented Nov 25, 2010

    I reviewed the patch.

    • I applied all the patchs(3 files).
    • Ran make and make html in the Doc directory.
    • Ran the test_wsgiref.py
    • Ran the python interpreter and typed the suggested code:

    >> from wsgiref.headers import Headers
    >> headers = Headers([])
    >> headers.add_header('Content-Type', 'text/plain')
    >> headers = Headers()
    >> headers.add_header('Content-Type', 'text/plain')
    >>

    • Read the documentation in a web browser.

    All in the revision 86742.

    @merwok
    Copy link
    Member

    merwok commented Nov 25, 2010

    LGTM.

    @SilentGhost SilentGhost mannequin added the stale Stale PR or inactive for long period of time. label Jan 15, 2011
    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Mar 6, 2011

    Here's the single-file patch against the revision.

    @pjeby
    Copy link
    Mannequin

    pjeby mannequin commented Mar 6, 2011

    Looks good to me.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Mar 13, 2011

    Looks good to me.
    Would you mind committing it then?

    @merwok
    Copy link
    Member

    merwok commented Mar 21, 2011

    I think the doc should say something like “default value is an empty list”, for maximum clarity.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jun 22, 2014

    This shouldn't be languishing, work has been committed against revision 86742 and there's another patch that apparently just needs a little tweak.

    @berkerpeksag
    Copy link
    Member

    The patch is not committed yet.

    $ ./python
    Python 3.5.0a0 (default:979aaa08c067+, Jun 19 2014, 13:01:36) 
    [GCC 4.6.3] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from wsgiref.headers import Headers
    >>> h = Headers()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: __init__() missing 1 required positional argument: 'headers'

    Here's an updated patch that addresses Éric's review (and without cosmetic changes to make review easier).

    @berkerpeksag berkerpeksag removed the stale Stale PR or inactive for long period of time. label Jun 22, 2014
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jun 22, 2014

    Just for the record clicking on "revision 86742" gives "Specified revision r86742 not found".

    @berkerpeksag
    Copy link
    Member

    Here's a new patch with a whatsnew entry. David, could you review the patch?

    @berkerpeksag
    Copy link
    Member

    Patch updated. If there's no objections, I'll commit this patch in the next couple of days.

    @berkerpeksag berkerpeksag self-assigned this Jul 1, 2014
    @bitdancer
    Copy link
    Member

    Since we don't need to avoid new language features, using the ternary would be better than using the or. This is exactly the sort of situation the ternary was introduced for: making sure the input really is None before applying the default.

    Otherwise the patch looks good.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 2, 2014

    New changeset a91f0d4a2381 by Berker Peksag in branch 'default':
    Issue bpo-5800: headers parameter of wsgiref.headers.Headers is now optional.
    http://hg.python.org/cpython/rev/a91f0d4a2381

    @berkerpeksag
    Copy link
    Member

    Thanks for the patch and review.

    @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
    Labels
    easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants