8000 bpo-34002: Minor efficiency and clarity improvements in email package. by selik · Pull Request #7999 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-34002: Minor efficiency and clarity improvements in email package. #7999

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

Merged
maxking merged 5 commits into from
Sep 20, 2019
Merged

bpo-34002: Minor efficiency and clarity improvements in email package. #7999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d8bc2c0
Check intersection of two sets explicitly
selik Jun 28, 2018
51e14f8
Change while/pop to a for-loop
selik Jun 28, 2018
c6168d1
Use set.intersection to check for intersections
selik Jun 29, 2018
b7e4749
Use not isdisjoint instead of intersection
selik Jun 29, 2018
4c61d82
Add NEWS item
maxking Sep 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions Lib/email/headerregistry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,9 @@ def addr_spec(self):
"""The addr_spec (username@domain) portion of the address, quoted
according to RFC 5322 rules, but with no Content Transfer Encoding.
"""
nameset = set(self.username)
if len(nameset) > len(nameset-parser.DOT_ATOM_ENDS):
lp = parser.quote_string(self.username)
else:
lp = self.username
lp = self.username
if not parser.DOT_ATOM_ENDS.isdisjoint(lp):
lp = parser.quote_string(lp)
if self.domain:
return lp + '@' + self.domain
if not lp:
Expand All @@ -86,11 +84,9 @@ def __repr__(self):
self.display_name, self.username, self.domain)

def __str__(self):
nameset = set(self.display_name)
if len(nameset) > len(nameset-parser.SPECIALS):
disp = parser.quote_string(self.display_name)
else:
disp = self.display_name
disp = self.display_name
if not parser.SPECIALS.isdisjoint(disp):
disp = parser.quote_string(disp)
if disp:
addr_spec = '' if self.addr_spec=='<>' else self.addr_spec
return "{} <{}>".format(disp, addr_spec)
Expand Down Expand Up @@ -141,10 +137,8 @@ def __str__(self):
if self.display_name is None and len(self.addresses)==1:
return str(self.addresses[0])
disp = self.display_name
if disp is not None:
nameset = set(disp)
if len(nameset) > len(nameset-parser.SPECIALS):
disp = parser.quote_string(disp)
if disp is not None and not parser.SPECIALS.isdisjoint(disp):
disp = parser.quote_string(disp)
adrstr = ", ".join(str(x) for x in self.addresses)
adrstr = ' ' + adrstr if adrstr else adrstr
return "{}:{};".format(disp, adrstr)
Expand Down
14 changes: 3 additions & 11 deletions Lib/email/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,21 +259,13 @@ def decode_params(params):

params is a sequence of 2-tuples containing (param name, string value).
"""
# Copy params so we don't mess with the original
params = params[:]
new_params = []
new_params = [params[0]]
Copy link
Member
@serhiy-storchaka serhiy-storchaka Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if params[0] is not a 2-tuple?

Copy link
Contributor Author
@selik selik Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possible for the first params element, though the others are implicitly checked by the unpacking for-loop.

Looking at the usage of decode_params, it's only use is in email.message._get_params_preserve which in turn has 3 call sites. All 3 calls unpack the elements as (k, v) pairs. The bad data would cause an immediate error, given how the method is used. The error would be the same as today, though the traceback would not be as deep.

If that's not satisfactory, I'll revert to:

key, value = params[0]
new_params = [(key, value)]

# Map parameter's name to a list of continuations. The values are a
# 3-tuple of the continuation number, the string value, and a flag
# specifying whether a particular segment is %-encoded.
rfc2231_params = {}
name, value = params.pop(0)
new_params.append((name, value))
while params:
name, value = params.pop(0)
if name.endswith('*'):
encoded = True
else:
encoded = False
for name, value in params[1:]:
Copy link
Member
@serhiy-storchaka serhiy-storchaka Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

params[1:] still creates a copy of the list.

Copy link
Contributor Author
@selik selik Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. The real benefit is avoiding repeated .pop(0).

Copy link
Member
@serhiy-storchaka serhiy-storchaka Jun 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the code can be made more optimal if avoid a copy.

Copy link
Member
@terryjreedy terryjreedy Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me that the final value of 'encoded' only depends on the final value of name. If so, the loop is equivalent to

if params:
    encoded = params[-1][0].endswith('*')

Otherwise, one can skip-iterate without copying with

it = iter(params)
try:
    next(it)
except StopIteration:
    pass
for name, value in it:
   ...

Copy link
Contributor Author
@selik selik Sep 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each iteration of the loop uses the encoded value. The iter and nexting would be a better technique.

it = iter(params)
new_params = [next(it)]
for name, value in it:
    ...

encoded = name.endswith('*')
value = unquote(value)
mo = rfc2231_continuation.match(name)
if mo:
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2019-09-19-19-58-33.bpo-34002.KBnaVX.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Improve efficiency in parts of email package by changing while-pop to a for
loop, using isdisjoint instead of set intersections.
0