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

Cookie.Morsel interface needs update #46464

Closed
AstraLuma mannequin opened this issue Feb 29, 2008 · 46 comments
Closed

Cookie.Morsel interface needs update #46464

AstraLuma mannequin opened this issue Feb 29, 2008 · 46 comments
Assignees
Labels
easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@AstraLuma
Copy link
Mannequin

AstraLuma mannequin commented Feb 29, 2008

BPO 2211
Nosy @loewis, @AstraLuma, @merwok, @abadger, @bitdancer, @berkerpeksag, @serhiy-storchaka, @demianbrecht
Files
  • issue2211.patch
  • issue2211_1.patch
  • issue2211_2.patch
  • issue2211_3.patch
  • issue2211_4.patch
  • issue2211_5.patch
  • issue2211_6.patch
  • http_cookies_morsel_pickle.patch
  • http_cookies_morsel_deprecate_set.patch
  • http_cookies_morsel_deprecated_set_1.patch
  • http_cookies_morsel_deprecated_set_2.patch
  • http_cookies_morsel_deprecated_set_3.patch
  • http_cookies_morsel_deprecated_set_4.patch
  • 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/bitdancer'
    closed_at = <Date 2015-03-31.23:53:33.612>
    created_at = <Date 2008-02-29.20:00:41.723>
    labels = ['easy', 'type-bug', 'library']
    title = 'Cookie.Morsel interface needs update'
    updated_at = <Date 2015-04-01.13:34:25.470>
    user = 'https://github.com/AstraLuma'

    bugs.python.org fields:

    activity = <Date 2015-04-01.13:34:25.470>
    actor = 'r.david.murray'
    assignee = 'r.david.murray'
    closed = True
    closed_date = <Date 2015-03-31.23:53:33.612>
    closer = 'demian.brecht'
    components = ['Library (Lib)']
    creation = <Date 2008-02-29.20:00:41.723>
    creator = 'AstraLuma'
    dependencies = []
    files = ['38170', '38410', '38432', '38466', '38476', '38509', '38524', '38540', '38584', '38688', '38690', '38695', '38729']
    hgrepos = []
    issue_num = 2211
    keywords = ['patch', 'easy']
    message_count = 46.0
    messages = ['63144', '63149', '63329', '63331', '64203', '104581', '235691', '235696', '236180', '236181', '237428', '237664', '237824', '237856', '238299', '238331', '238387', '238389', '238395', '238401', '238448', '238456', '238565', '238603', '238616', '238684', '238923', '238924', '238925', '239259', '239260', '239262', '239269', '239272', '239286', '239521', '239522', '239523', '239606', '239615', '239616', '239617', '239623', '239672', '239765', '239807']
    nosy_count = 11.0
    nosy_names = ['loewis', 'jafo', 'AstraLuma', 'eric.araujo', 'a.badger', 'r.david.murray', 'BreamoreBoy', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'demian.brecht']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue2211'
    versions = ['Python 3.5']

    @AstraLuma
    Copy link
    Mannequin Author

    AstraLuma mannequin commented Feb 29, 2008

    Cookie.Morsel lacks in properly overloading dict methods, amongst other
    things:

    • .__eq__() does not take .key and .value into account
    • .copy() returns a dict, not a Morsel
    • .update() allows invalid attributes in, eg
      Morsel().update({'eggs':'spam'}) works but Morsel()['eggs'] = 'spam' fails
    • .__len__() includes unset keys (why does it set them to an empty string?)
    • .__repr__() doesn't print out attributes (path, comment, etc)
    • Handling of unicode is fuzzy at best: try Morsel().set(u'eggs\u00bf',
      u'\u00f1', u'\00f1'.encode('utf-8'))
    • Specifying the idmap and translate function in .set() seems wrong
    • Setting .key doesn't check against invalid keys (eg, reserved names)
    • The entire Morsel class is undocumented
    • There is no way to automatically sync .value and .coded_value

    @AstraLuma AstraLuma mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 29, 2008
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 29, 2008

    Would you be interested to work on a patch?

    @AstraLuma
    Copy link
    Mannequin Author

    AstraLuma mannequin commented Mar 6, 2008

    Sure, I'll do that.

    @AstraLuma
    Copy link
    Mannequin Author

    AstraLuma mannequin commented Mar 6, 2008

    • Should be backwards compatible with people who actually use Morsel
    • Didn't even attempt to document it
    • Instead of having the Morsel dict being filled initially, unset
      attributes are, well, unset
    • .key, .value, and .coded_value are now property()s
    • .value is considered the actual value (eg, .coded_value is ignore for
      __eq__())

    @jafo
    Copy link
    Mannequin

    jafo mannequin commented Mar 20, 2008

    I'm going to push this to pending until you can get a patch. Thanks, Jamie.

    @bitdancer
    Copy link
    Member

    bitdancer commented Apr 29, 2010

    This looks like it would be a worthwhile cleanup, and the issue contains useful info to that end, so I'm marking it languishing rather than closing it, in the hopes that someone will pick it up some day. I also think this could be a nice bug day task for someone, (though it might take all day :), so I'm adding the easy keyword.

    (From the sounds of Jamie's second post, he may have thought he uploaded a patch, but didn't...)

    Note: I suspect the logic of having 'unset' keys return an empty string is that a Morsel is supposed to have a "fixed set" of keys (and thus those keys should always be there). I'm not sure that should be changed.

    @bitdancer bitdancer added easy stale Stale PR or inactive for long period of time. labels Apr 29, 2010
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Feb 10, 2015

    @demian is this of any interest to you?

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Feb 10, 2015

    @mark: Sure, but not super high priority. Thanks for pointing it out.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Feb 18, 2015

    The attached patch should cover the implementation/test aspects of this issue. I'll work on the documentation next.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Feb 18, 2015

    Never mind, Morsel /is/ documented (at least in Docs). I imagine that the OP was after more detailed docstrings, but I don't believe they'd add much value.

    The patch should be good to go as-is.

    @serhiy-storchaka serhiy-storchaka removed the stale Stale PR or inactive for long period of time. label Mar 7, 2015
    @serhiy-storchaka serhiy-storchaka self-assigned this Mar 7, 2015
    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 7, 2015

    Added comments on Rietveld.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 9, 2015

    New patch addresses most review comments. Thanks for the review Serhiy.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 10, 2015

    Latest patch should address all comments.

    @berkerpeksag
    Copy link
    Member

    berkerpeksag commented Mar 11, 2015

    Serhiy already reviewed the latest patch. Just one more comment: The deprecated API should be documented in Doc/whatsnew/3.5.rst and Doc/library/http.cookies.rst.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 17, 2015

    Here is a patch with extended and unified tests. Also fixed one bug. I have left comments about some changes on Rietveld.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 17, 2015

    Thanks for the updates Serhiy. All look good to me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 18, 2015

    New changeset 88e1151e8e02 by Serhiy Storchaka in branch 'default':
    Issue bpo-2211: Updated the implementation of the http.cookies.Morsel class.
    https://hg.python.org/cpython/rev/88e1151e8e02

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 18, 2015

    Renamed __copy__ to copy, because copy.copy() doesn't need these changes and original report was about the copy() method.

    Thank you for your contribution and for your responsiveness Demian.

    @vstinner
    Copy link
    Member

    vstinner commented Mar 18, 2015

    New changeset 88e1151e8e02 by Serhiy Storchaka in branch 'default':
    Issue bpo-2211: Updated the implementation of the http.cookies.Morsel class.
    https://hg.python.org/cpython/rev/88e1151e8e02

    I don't understand why, but test_pickle started to fail with this change.

    See 3.x buildbots for errors. Example:
    http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/5858/steps/test/logs/stdio

    @vstinner vstinner reopened this Mar 18, 2015
    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 18, 2015

    Thanks Victor.

    This patch should fix pickling/unpickling.

    Interesting, we should check all other cases when instance attribute was converted to a property. They are potentially break pickle compatibility.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 18, 2015

    Thanks for the follow up Serhiy, LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 18, 2015

    New changeset d68cc584bc7d by Serhiy Storchaka in branch 'default':
    Restored backward compatibility of pickling http.cookies.Morsel. It was
    https://hg.python.org/cpython/rev/d68cc584bc7d

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 19, 2015

    The change to the signature of set is backward incompatible and should have a deprecation warning instead (yes, I know it isn't documented). I'd even be OK with the value being ignored, but the signature shouldn't change.

    There are a number of API/behavior changes noted in the NEWS which IMO need a versionchanged in the docs and an entry in whatsnew.

    @bitdancer bitdancer reopened this Mar 19, 2015
    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 20, 2015

    The attached patch reverts back to the old set() API, ignoring the parameter and adding a deprecation warning. I've also added a few versionchanged and deprecated tags in the Morsel docs. Other than the deprecation note in whatsnew/3.5.rst that was added, I'm not entirely sure what you're suggesting should be added (I'm also not overly familiar with what qualifies for "what's new" additions in the various categories without causing unnecessary noise).

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 20, 2015

    In 2.7 Morsel.set is declared as:

        def set(self, key, val, coded_val,
                LegalChars=_LegalChars,
                idmap=_idmap, translate=string.translate):

    Undocumented parameters idmap and translate were removed without deprecation. idmap was removed in 14b65de9b798, translate was removed in 99027c2b3fd2 when their became unnecessary. Now LegalChars is unnecessary.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 20, 2015

    idmap was removed in 14b65de9b798, translate was removed in 99027c2b3fd2 when their became unnecessary.

    I'd venture to say they slipped through the cracks. Following the deprecation procedure here would be favourable IMHO as to give users a sufficient heads up that their will be a breakage upcoming, however unlikely that the parameter is actually in use in practice. There's nothing lost by marking it deprecated and removing it for 3.6(+).

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 22, 2015

    It looks like both of those changes were part of the python3 transition, and thus not subject to our backward compatibility rules. (Which, it should be noted, we applied rather more loosely in python 3.1, 3.2, and even to a smaller extent in 3.3).

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 22, 2015

    Demien, your patch looks good to me except that it would be better to consolidate the three adjacent versionchanged entries into one.

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 22, 2015

    Oh, and the additional what's new text would in this case be pretty much the contents of that multi-line versionchanged entry, as a bullet item in the 'porting' section. That is, they are behavior changes that are closer to API fixes than new features, and so don't need to be entered into the 'module improvements' section, but should be mentioned as possible porting issues (all this is IMO, of course).

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 25, 2015

    it would be better to consolidate the three adjacent versionchanged entries into one

    I created the three versionchanged items because there's no visual distinction between the second and third lines and the rest of the doc tests in the rendered output. I've changed it as suggested as you're obviously more familiar with other instances of this, but I would still suggest reverting it to the three individual points if this isn't a docs best practice (or updating the versionchanged CSS class to have some visual distinction between it and the rest of the docs).

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 25, 2015

    The body of the versionchanged has to be a single paragraph (no blank lines). Usually we start each sentence on a newline in the source, but it comes out rendered as a single flowed paragraph. Perhaps eventually someone will add support to Sphinx for unordered lists in a directive, but a single paragraph is good enough.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 25, 2015

    FWIW, I created bpo-23778 to address the indentation issue.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 25, 2015

    Problem (pretty much) solved. Nested unordered lists are supported. I've updated the versionchanged information to use the list.

    @berkerpeksag
    Copy link
    Member

    berkerpeksag commented Mar 25, 2015

    I think David's suggestion in msg239260 was good enough for now :) You'll need to create a custom versionchanged directive to generate a valid and semantic markup for the usage in http_cookies_morsel_deprecated_set_2.patch. I also left a couple comments on Rietveld.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 25, 2015

    Updated patch should address review comments. I did run with David's suggestion as I also noticed that my initial assessment was wrong (thanks for the note/review Berker).

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 29, 2015

    I made several changes to the patch.

    The whatsnew docs didn't format correctly: there's an extra space on all the lines after the first (a three space indent is apparently required there which I don't understand, but it is also our standard for docs). Also none of the links work, because in the whatsnew document (unlike in the doc page itself) you have to fully qualify the names (eg: ~http.cookies.Morsel to display 'Morsel' and link to the right place). The line about LegalChars was in the wrong section. I rewrote both as a single bullet item with working links.

    The 'update' and 'copy' methods were not documented. I added them and moved the relevant 'versionchanged' phrases under the newly documented methods.

    In addition, I decided that the deprecation for key, value, and coded_value would be clearer if phrased as : "assignment to XXX; use set instead".

    A couple of other notes: while functionally it doesn't (currently) make any difference to Sphinx if you use :func: vs :meth:, :meth: is the correct one for all these cases. And you had an extra blank line after the versionchanged directive.

    I've uploaded the changeset I applied as a diff so that you can more easily see what I changed if you wish.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 29, 2015

    New changeset 09f22b5d6cea by R David Murray in branch 'default':
    bpo-2211: properly document the Morsel behavior changes.
    https://hg.python.org/cpython/rev/09f22b5d6cea

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 29, 2015

    Now, anyone want to place bets on someone getting bitten by the new errors that update will raise? :)

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 30, 2015

    Did you noticed my comments on Rietveld about setdefault() and the :meth: role?

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 30, 2015

    No, sorry, I currently don't always notice reviews unless they are mentioned on the tracker. I will take a look.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 30, 2015

    New changeset a70ca6f35327 by R David Murray in branch 'default':
    bpo-2211: Fix typo, address missed review comment.
    https://hg.python.org/cpython/rev/a70ca6f35327

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 30, 2015

    Serhiy: I had already dealt with all the comments in my revisions in one way or another, except for the one about setdefault. I've added documentation for that now, too. Sorry for not posting everything for review first...if you see anything you object to in the commits. please let me know.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 30, 2015

    Thanks for following up on this David. The changes you've made all look good to me.

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Mar 31, 2015

    Thank you David, all LGTM.

    I noted that the :func: role sometimes is used for methods in the whatsnew file (and perhaps in other rst files). Perhaps it should be changed to :meth:. But this is other issue.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Mar 31, 2015

    Set as closed, assuming there's no reason to keep this issue open.

    @demianbrecht demianbrecht mannequin closed this as completed Mar 31, 2015
    @bitdancer
    Copy link
    Member

    bitdancer commented Apr 1, 2015

    Heh, I thought it was closed. I guess my eyes were distracted by the 'fixed' resolution.

    @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-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants