From bd6332d245a4ffbd5a819c5555fd862e1e19789c Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Sun, 8 Jul 2018 15:27:52 -0700 Subject: [PATCH] PEP 572: get rid of side-by-side tables; drop unnecessary TODO (#708) Alas, the side-by-side tables caused horribly misindented code. --- pep-0572.rst | 205 +++++++++++++++++++++++++++------------------------ 1 file changed, 107 insertions(+), 98 deletions(-) diff --git a/pep-0572.rst b/pep-0572.rst index 50c77afe6..b9f58d6f6 100644 --- a/pep-0572.rst +++ b/pep-0572.rst @@ -166,16 +166,8 @@ in order to avoid ambiguities or user confusion: foo(x = y := f(x)) # INVALID - This rule is included to disallow excessively confusing code. - -- TODO: Should we disallow using keyword arguments and top level - assignment expressions in the same call? E.g.:: - - # Should these be invalid? - foo(x=0, y := f(0)) - bar(x := 0, y = f(x)) - - Regardless, ``foo(x := 0)`` should probably be valid (see below). + This rule is included to disallow excessively confusing code, and + because parsing keyword arguments is complex enough already. - Assignment expressions (even parenthesized or occurring inside other constructs) are prohibited in function default values. For example, @@ -294,7 +286,7 @@ Some examples to clarify what's technically valid or invalid:: # Valid len(lines := f.readlines()) - # Valid (TODO: Should this be disallowed?) + # Valid foo(x := 3, cat='vector') # INVALID @@ -366,121 +358,136 @@ Examples from the Python standard library site.py ^^^^^^^ -+-----------------------------------------------------+------------------------------------------------------------+ -| Current code | Replaced with | -+-----------------------------------------------------+------------------------------------------------------------+ -| | | -| :: | :: | -| | | -| env_base = os.environ.get("PYTHONUSERBASE", None) | if env_base := os.environ.get("PYTHONUSERBASE", None): | -| if env_base: | return env_base | -| return env_base | | -| | | -+-----------------------------------------------------+------------------------------------------------------------+ - *env_base* is only used on these lines, putting its assignment on the if moves it as the "header" of the block. +- Current:: + + env_base = os.environ.get("PYTHONUSERBASE", None) + if env_base: + return env_base + +- Improved:: + + if env_base := os.environ.get("PYTHONUSERBASE", None): + return env_base + _pydecimal.py ^^^^^^^^^^^^^ -+----------------------------------------------+-----------------------------------------------------------------------+ -| Current code | Replaced with | -+----------------------------------------------+-----------------------------------------------------------------------+ -| | | -| :: | :: | -| | | -| if self._is_special: | if self._is_special and (ans := self._check_nans(context=context)): | -| ans = self._check_nans(context=context) | return ans | -| if ans: | | -| return ans | | -| | | -+----------------------------------------------+-----------------------------------------------------------------------+ Avoid nested if and remove one indentation level. +- Current:: + + if self._is_special: + ans = self._check_nans(context=context) + if ans: + return ans + +- Improved:: + + if self._is_special and (ans := self._check_nans(context=context)): + return ans + copy.py ^^^^^^^ +Code looks more regular and avoid multiple nested if. (See Appendix A for the origin of this example.) -+---------------------------------------------------------------+---------------------------------------------------------------+ -| Current code | Replaced with | -+---------------------------------------------------------------+---------------------------------------------------------------+ -| | | -| :: | :: | -| | | -| reductor = dispatch_table.get(cls) | if reductor := dispatch_table.get(cls): | -| if reductor: | rv = reductor(x) | -| rv = reductor(x) | elif reductor := getattr(x, "__reduce_ex__", None): | -| else: | rv = reductor(4) | -| reductor = getattr(x, "__reduce_ex__", None) | elif reductor := getattr(x, "__reduce__", None): | -| if reductor: | rv = reductor() | -| rv = reductor(4) | else: | -| else: | raise Error("un(deep)copyable object of type %s" % cls) | -| reductor = getattr(x, "__reduce__", None) | | -| if reductor: | | -| rv = reductor() | | -| else: | | -| raise Error( | | -| "un(deep)copyable object of type %s" % cls) | | -+---------------------------------------------------------------+---------------------------------------------------------------+ +- Current:: -Code looks more regular and avoid multiple nested if. + reductor = dispatch_table.get(cls) + if reductor: + rv = reductor(x) + else: + reductor = getattr(x, "__reduce_ex__", None) + if reductor: + rv = reductor(4) + else: + reductor = getattr(x, "__reduce__", None) + if reductor: + rv = reductor() + else: + raise Error( + "un(deep)copyable object of type %s" % cls) + +- Improved:: + + if reductor := dispatch_table.get(cls): + rv = reductor(x) + elif reductor := getattr(x, "__reduce_ex__", None): + rv = reductor(4) + elif reductor := getattr(x, "__reduce__", None): + rv = reductor() + else: + raise Error("un(deep)copyable object of type %s" % cls) datetime.py ^^^^^^^^^^^ -+-----------------------------------------------------+-----------------------------------------------------+ -| Current code | Replaced with | -+-----------------------------------------------------+-----------------------------------------------------+ -| | | -| :: | :: | -| | | -| s = _format_time(self._hour, self._minute, | s = _format_time(self._hour, self._minute, | -| self._second, self._microsecond, | self._second, self._microsecond, | -| timespec) | timespec) | -| tz = self._tzstr() | if tz := self._tzstr(): | -| if tz: | s += tz | -| s += tz | return s | -| return s | | -| | | -+-----------------------------------------------------+-----------------------------------------------------+ - *tz* is only used for ``s += tz``, moving its assignment inside the if helps to show its scope. +- Current:: + + s = _format_time(self._hour, self._minute, + self._second, self._microsecond, + timespec) + tz = self._tzstr() + if tz: + s += tz + return s + +- Improved:: + + s = _format_time(self._hour, self._minute, + self._second, self._microsecond, + timespec) + if tz := self._tzstr(): + s += tz + return s + sysconfig.py ^^^^^^^^^^^^ -+------------------------------------+-----------------------------------------+ -| Current code | Replaced with | -+------------------------------------+-----------------------------------------+ -| | | -| :: | :: | -| | | -| while True: | while line := fp.readline(): | -| line = fp.readline() | if m := define_rx.match(line): | -| if not line: | n, v = m.group(1, 2) | -| break | try: | -| m = define_rx.match(line) | v = int(v) | -| if m: | except ValueError: | -| n, v = m.group(1, 2) | pass | -| try: | vars[n] = v | -| v = int(v) | elif m := undef_rx.match(line): | -| except ValueError: | vars[m.group(1)] = 0 | -| pass | | -| vars[n] = v | | -| else: | | -| m = undef_rx.match(line) | | -| if m: | | -| vars[m.group(1)] = 0 | | -| | | -+------------------------------------+-----------------------------------------+ - Calling ``fp.readline()`` in the ``while`` condition and calling ``.match()`` on the if lines make the code more compact without making it harder to understand. +- Current:: + + while True: + line = fp.readline() + if not line: + break + m = define_rx.match(line) + if m: + n, v = m.group(1, 2) + try: + v = int(v) + except ValueError: + pass + vars[n] = v + else: + m = undef_rx.match(line) + if m: + vars[m.group(1)] = 0 + +- Improved:: + + while line := fp.readline(): + if m := define_rx.match(line): + n, v = m.group(1, 2) + try: + v = int(v) + except ValueError: + pass + vars[n] = v + elif m := undef_rx.match(line): + vars[m.group(1)] = 0 + + Simplifying list comprehensions ------------------------------- @@ -613,6 +620,8 @@ Broadly the same semantics as the current proposal, but spelled differently. confusion or require special-casing (eg to forbid assignment within the headers of these statements). + TODO: Explain more why this is bad, since this keeps coming up (it's also readability) + 2. ``EXPR -> NAME``:: stuff = [[f(x) -> y, x/y] for x in range(5)]