tornado icon indicating copy to clipboard operation
tornado copied to clipboard

Increase the usage of augmented assignment statements

Open elfring opened this issue 4 years ago • 2 comments

:eyes: Some source code analysis tools can help to find opportunities for improving software components. :thought_balloon: I propose to increase the usage of augmented assignment statements accordingly.

diff --git a/demos/tcpecho/server.py b/demos/tcpecho/server.py
index ebe683cf..bae07921 100755
--- a/demos/tcpecho/server.py
+++ b/demos/tcpecho/server.py
@@ -19,7 +19,7 @@ class EchoServer(TCPServer):
                 data = yield stream.read_until(b"\n")
                 logger.info("Received bytes: %s", data)
                 if not data.endswith(b"\n"):
-                    data = data + b"\n"
+                    data += b"\n"
                 yield stream.write(data)
             except StreamClosedError:
                 logger.warning("Lost client at host %s", address[0])
diff --git a/tornado/iostream.py b/tornado/iostream.py
index 930f73d6..206f6afb 100644
--- a/tornado/iostream.py
+++ b/tornado/iostream.py
@@ -1059,7 +1059,7 @@ class BaseIOStream(object):
             self._state = ioloop.IOLoop.ERROR | state
             self.io_loop.add_handler(self.fileno(), self._handle_events, self._state)
         elif not self._state & state:
-            self._state = self._state | state
+            self._state |= state
             self.io_loop.update_handler(self.fileno(), self._state)
 
     def _is_connreset(self, exc: BaseException) -> bool:
diff --git a/tornado/platform/asyncio.py b/tornado/platform/asyncio.py
index 5e9c776d..070a3159 100644
--- a/tornado/platform/asyncio.py
+++ b/tornado/platform/asyncio.py
@@ -555,7 +555,7 @@ class AddThreadSelectorEventLoop(asyncio.AbstractEventLoop):
                 # This pattern is also used in
                 # https://github.com/python/cpython/blob/v3.8.0/Lib/selectors.py#L312-L317
                 rs, ws, xs = select.select(to_read, to_write, to_write)
-                ws = ws + xs
+                ws += xs
             except OSError as e:
                 # After remove_reader or remove_writer is called, the file
                 # descriptor may subsequently be closed on the event loop
diff --git a/tornado/util.py b/tornado/util.py
index b69500e2..2fc3a0e2 100644
--- a/tornado/util.py
+++ b/tornado/util.py
@@ -442,7 +442,7 @@ def _websocket_mask_python(mask: bytes, data: bytes) -> bytes:
     mask_arr = array.array("B", mask)
     unmasked_arr = array.array("B", data)
     for i in range(len(data)):
-        unmasked_arr[i] = unmasked_arr[i] ^ mask_arr[i % 4]
+        unmasked_arr[i] ^= mask_arr[i % 4]
     return unmasked_arr.tobytes()
 
 

elfring avatar Nov 13 '21 17:11 elfring

Why is increasing usage of augmented assignment statements desirable?

This is not always semantically equivalent. += on a list mutates the list while the non-augmented assignment does not. Of these examples, the one in asyncio.py operates on a list-like object, which is documented only as being a "subset of the first three arguments". Could it be some kind of view that passes modifications through to the inputs? (not in the current implementation, but is there ever any chance it could change?)

The changes are most likely fine, even the one in asyncio.py. But what is the reason for taking the time to investigate and make the change?

bdarnell avatar Nov 15 '21 02:11 bdarnell

Why is increasing usage of augmented assignment statements desirable?

Do you find the explanation by the Python enhancement proposal 203 (from 2000-07-13) reasonable? :thinking:

This is not always semantically equivalent.

I suggest to reconsider such a wording. :point_right: A value will be assigned in the documented way.

But what is the reason for taking the time to investigate and make the change?

:thought_balloon: Can the run time characteristics be influenced in desirable directions also for this software?

elfring avatar Nov 15 '21 11:11 elfring

Do you find the explanation by the Python enhancement proposal 203 (from 2000-07-13) reasonable?

I like augmented assignment operators and use them. All of Tornado was written in a time that augmented assignment statements were available. So when there's a case that could have used augmented assignment and didn't, sometimes there's a reason. Sometimes there's no reason and it's totally fine to change, but it doesn't seem like there's enough benefit to making the change to make it worth thinking about.

bdarnell avatar Aug 28 '22 18:08 bdarnell