classification
Title: Cookie.Morsel interface needs update
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: r.david.murray Nosy List: BreamoreBoy, a.badger, astronouth7303, berker.peksag, demian.brecht, eric.araujo, jafo, loewis, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: easy, patch

Created on 2008-02-29 20:00 by astronouth7303, last changed 2015-04-01 13:34 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
issue2211.patch demian.brecht, 2015-02-18 15:57 review
issue2211_1.patch demian.brecht, 2015-03-09 15:38 review
issue2211_2.patch demian.brecht, 2015-03-10 23:24 review
issue2211_3.patch demian.brecht, 2015-03-13 06:05 review
issue2211_4.patch demian.brecht, 2015-03-13 21:18 review
issue2211_5.patch demian.brecht, 2015-03-16 14:23 review
issue2211_6.patch serhiy.storchaka, 2015-03-17 14:53 review
http_cookies_morsel_pickle.patch serhiy.storchaka, 2015-03-18 10:38 review
http_cookies_morsel_deprecate_set.patch demian.brecht, 2015-03-20 05:14 review
http_cookies_morsel_deprecated_set_1.patch demian.brecht, 2015-03-25 14:51 review
http_cookies_morsel_deprecated_set_2.patch demian.brecht, 2015-03-25 16:24 review
http_cookies_morsel_deprecated_set_3.patch demian.brecht, 2015-03-25 23:10 review
http_cookies_morsel_deprecated_set_4.patch r.david.murray, 2015-03-29 21:09 review
Messages (46)
msg63144 - (view) Author: Jamie Bliss (astronouth7303) Date: 2008-02-29 20:00
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
msg63149 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-02-29 23:08
Would you be interested to work on a patch?
msg63329 - (view) Author: Jamie Bliss (astronouth7303) Date: 2008-03-06 18:36
Sure, I'll do that.
msg63331 - (view) Author: Jamie Bliss (astronouth7303) Date: 2008-03-06 19:14
* 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__())
msg64203 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2008-03-20 20:32
I'm going to push this to pending until you can get a patch.  Thanks, Jamie.
msg104581 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-04-29 20:21
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.
msg235691 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-02-10 15:34
@Demian is this of any interest to you?
msg235696 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-10 16:16
@Mark: Sure, but not super high priority. Thanks for pointing it out.
msg236180 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-18 15:57
The attached patch should cover the implementation/test aspects of this issue. I'll work on the documentation next.
msg236181 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-18 16:12
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.
msg237428 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-07 09:02
Added comments on Rietveld.
msg237664 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-09 15:38
New patch addresses most review comments. Thanks for the review Serhiy.
msg237824 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-10 23:24
Latest patch should address all comments.
msg237856 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-03-11 09:29
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.
msg238299 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-17 14:53
Here is a patch with extended and unified tests. Also fixed one bug. I have left comments about some changes on Rietveld.
msg238331 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-17 19:17
Thanks for the updates Serhiy. All look good to me.
msg238387 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-18 09:00
New changeset 88e1151e8e02 by Serhiy Storchaka in branch 'default':
Issue #2211: Updated the implementation of the http.cookies.Morsel class.
https://hg.python.org/cpython/rev/88e1151e8e02
msg238389 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-18 09:05
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.
msg238395 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-18 09:51
> New changeset 88e1151e8e02 by Serhiy Storchaka in branch 'default':
> Issue #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
msg238401 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-18 10:38
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.
msg238448 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-18 14:28
Thanks for the follow up Serhiy, LGTM.
msg238456 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-18 16:04
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
msg238565 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-19 21:56
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.
msg238603 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-20 05:14
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).
msg238616 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-20 08:29
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.
msg238684 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-20 14:38
> 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(+).
msg238923 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-22 17:22
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).
msg238924 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-22 17:25
Demien, your patch looks good to me except that it would be better to consolidate the three adjacent versionchanged entries into one.
msg238925 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-22 17:28
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).
msg239259 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-25 14:51
> 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).
msg239260 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-25 15:09
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.
msg239262 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-25 15:24
FWIW, I created #23778 to address the indentation issue.
msg239269 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-25 16:24
Problem (pretty much) solved. Nested unordered lists are supported. I've updated the versionchanged information to use the list.
msg239272 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-03-25 16:33
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.
msg239286 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-25 23:10
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).
msg239521 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-29 21:09
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.
msg239522 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-29 21:10
New changeset 09f22b5d6cea by R David Murray in branch 'default':
#2211: properly document the Morsel behavior changes.
https://hg.python.org/cpython/rev/09f22b5d6cea
msg239523 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-29 21:11
Now, anyone want to place bets on someone getting bitten by the new errors that update will raise? :)
msg239606 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-30 13:13
Did you noticed my comments on Rietveld about setdefault() and the :meth: role?
msg239615 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-30 15:09
No, sorry, I currently don't always notice reviews unless they are mentioned on the tracker.  I will take a look.
msg239616 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-30 15:54
New changeset a70ca6f35327 by R David Murray in branch 'default':
#2211: Fix typo, address missed review comment.
https://hg.python.org/cpython/rev/a70ca6f35327
msg239617 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-03-30 15:56
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.
msg239623 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-30 16:38
Thanks for following up on this David. The changes you've made all look good to me.
msg239672 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-31 06:09
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.
msg239765 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-31 23:53
Set as closed, assuming there's no reason to keep this issue open.
msg239807 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-01 13:34
Heh, I thought it was closed.  I guess my eyes were distracted by the 'fixed' resolution.
History
Date User Action Args
2015-04-01 13:34:25r.david.murraysetmessages: + msg239807
2015-03-31 23:53:33demian.brechtsetstatus: open -> closed

messages: + msg239765
2015-03-31 06:09:15serhiy.storchakasetmessages: + msg239672
2015-03-30 16:38:16demian.brechtsetmessages: + msg239623
2015-03-30 15:56:08r.david.murraysetmessages: + msg239617
2015-03-30 15:54:59python-devsetmessages: + msg239616
2015-03-30 15:09:15r.david.murraysetmessages: + msg239615
2015-03-30 13:13:12serhiy.storchakasetassignee: serhiy.storchaka -> r.david.murray
messages: + msg239606
2015-03-29 21:11:46r.david.murraysetmessages: + msg239523
2015-03-29 21:10:12python-devsetmessages: + msg239522
2015-03-29 21:09:59r.david.murraysetfiles: + http_cookies_morsel_deprecated_set_4.patch

messages: + msg239521
2015-03-25 23:10:36demian.brechtsetfiles: + http_cookies_morsel_deprecated_set_3.patch

messages: + msg239286
2015-03-25 16:33:04berker.peksagsetmessages: + msg239272
2015-03-25 16:24:29demian.brechtsetfiles: + http_cookies_morsel_deprecated_set_2.patch

messages: + msg239269
2015-03-25 15:24:20demian.brechtsetmessages: + msg239262
2015-03-25 15:09:17r.david.murraysetmessages: + msg239260
2015-03-25 14:51:26demian.brechtsetfiles: + http_cookies_morsel_deprecated_set_1.patch

messages: + msg239259
2015-03-22 17:28:40r.david.murraysetmessages: + msg238925
2015-03-22 17:25:53r.david.murraysetmessages: + msg238924
2015-03-22 17:22:32r.david.murraysetmessages: + msg238923
2015-03-20 14:38:46demian.brechtsetmessages: + msg238684
2015-03-20 08:42:54vstinnersetnosy: - vstinner
2015-03-20 08:29:35serhiy.storchakasetmessages: + msg238616
2015-03-20 05:14:40demian.brechtsetfiles: + http_cookies_morsel_deprecate_set.patch

messages: + msg238603
2015-03-19 21:56:43r.david.murraysetstatus: closed -> open

messages: + msg238565
2015-03-18 17:36:25serhiy.storchakasetstatus: open -> closed
resolution: fixed
2015-03-18 16:04:19python-devsetmessages: + msg238456
2015-03-18 14:28:34demian.brechtsetmessages: + msg238448
2015-03-18 10:38:20serhiy.storchakasetfiles: + http_cookies_morsel_pickle.patch

messages: + msg238401
2015-03-18 09:51:24vstinnersetstatus: closed -> open

nosy: + vstinner
messages: + msg238395

resolution: fixed -> (no value)
2015-03-18 09:05:18serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg238389

stage: commit review -> resolved
2015-03-18 09:00:48python-devsetnosy: + python-dev
messages: + msg238387
2015-03-17 21:22:00demian.brechtsetstage: patch review -> commit review
2015-03-17 19:17:38demian.brechtsetmessages: + msg238331
2015-03-17 14:53:28serhiy.storchakasetfiles: + issue2211_6.patch

messages: + msg238299
2015-03-16 14:23:36demian.brechtsetfiles: + issue2211_5.patch
2015-03-13 21:18:23demian.brechtsetfiles: + issue2211_4.patch
2015-03-13 06:05:16demian.brechtsetfiles: + issue2211_3.patch
2015-03-11 09:29:43berker.peksagsetmessages: + msg237856
2015-03-10 23:24:29demian.brechtsetfiles: + issue2211_2.patch

messages: + msg237824
2015-03-09 15:38:59demian.brechtsetfiles: + issue2211_1.patch

messages: + msg237664
2015-03-07 09:02:20serhiy.storchakasetmessages: + msg237428
2015-03-07 09:01:58serhiy.storchakasetstatus: languishing -> open
assignee: serhiy.storchaka

nosy: + serhiy.storchaka
2015-02-19 04:00:03berker.peksagsetnosy: + berker.peksag

stage: test needed -> patch review
2015-02-18 16:12:14demian.brechtsetmessages: + msg236181
2015-02-18 15:57:26demian.brechtsetfiles: + issue2211.patch
keywords: + patch
messages: + msg236180

versions: + Python 3.5, - Python 3.2
2015-02-10 16:16:22demian.brechtsetmessages: + msg235696
2015-02-10 15:34:34BreamoreBoysetnosy: + demian.brecht, BreamoreBoy
messages: + msg235691
2010-05-16 19:36:28eric.araujosetnosy: + eric.araujo
2010-04-29 20:21:42r.david.murraysetstatus: pending -> languishing

versions: + Python 3.2, - Python 2.6, Python 2.5
keywords: + easy
nosy: + r.david.murray

messages: + msg104581
stage: test needed
2008-08-04 21:39:33a.badgersetnosy: + a.badger
2008-03-20 20:32:28jafosetstatus: open -> pending
2008-03-20 20:32:20jafosetpriority: normal
nosy: + jafo
messages: + msg64203
2008-03-06 19:14:53astronouth7303setmessages: + msg63331
2008-03-06 18:36:14astronouth7303setmessages: + msg63329
2008-02-29 23:08:02loewissetnosy: + loewis
messages: + msg63149
2008-02-29 20:00:41astronouth7303create