[GitHub] [airflow] potiuk commented on a change in pull request #5786: [AIRFLOW-5170] [AIRFLOW-5256] Consistent licences for python files and related pylint fixes

[GitBox <gi...@apache.org>]

potiuk commented on a change in pull request #5786:  [AIRFLOW-5170] [AIRFLOW-5256] Consistent licences for python files and related pylint fixes
URL: https://github.com/apache/airflow/pull/5786#discussion_r319969258
 
 

 ##########
 File path: airflow/migrations/versions/33ae817a1ff4_add_kubernetes_resource_checkpointing.py
 ##########
 @@ -44,11 +44,11 @@ def upgrade():
     conn = op.get_bind()
 
     # alembic creates an invalid SQL for mssql and mysql dialects
-    if conn.dialect.name in ("mysql"):
+    if conn.dialect.name in {"mysql"}:
 
 Review comment:
   Yeah. The conditions were actually wrong so I fixed them :). 
   
   The "in" operator for strings tests for substring match:
   ```
   For the string and bytes types, x in y is True if and only if x is a substring of y. An equivalent test is y.find(x) != -1. Empty strings are always considered to be a substring of any other string, so "" in "abc" will return True.
   ```
   
   The original condition `conn.dialect.name in ("mysql")` is equivalent to `conn.dialect.name in "mysql"` - the parenthesis are superfluous and they suggest testing `in` against a tuple. I suspect that the original test was actually with the tuple originally ("mysql", "mssql") and someone split the if to mysql/mssql later on but did not notice that the tuple was removed along the way.
   
   Good test in this case would have been `conn.dialect.name in ("mysql",)` (note the coma), but because it is so easy miss the coma and it's not at all obvious, I prefer matching against set rather than tuple (there is no way to make similar mistake in set) - that's why I modified it to use sets in both cases.
   
   It was working fine before - of course - as substrings tested were not ambiguous across different dialects but nevertheless, I think it's worth to fix it (but It would be too much overhead to create a separate issue/commit just to fix those). 
   
   What do you think @feluelle ?
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services