Enable pylint rule to check for missing commas #12882
Conversation
Using implicit string concatenation in lists, even across multiple lines is almost always a sign of a bug, and Pylint has a check for this we can enable.
ashb
added
the
full tests needed
|
The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest master or amend the last commit of the PR, and push it with --force-with-lease. |
github-actions
bot
added
the
okay to merge
|
OH YEA! |
|
I will run i locally to check /fix. Maybe faster than in GA |
|
I didn't test what else this might catch (i.e. what we need to fix) but did test that it would catch the specific case that bit us. |
|
Running |
|
Oh that's a good idea. This was what I checked: |
|
Those are the errors: fixing them in my fixup: ************* Module tests.providers.amazon.aws.sensors.test_step_function_executiontests/providers/amazon/aws/sensors/test_step_function_execution.py:30:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat) |
|
BTW: https://www.python.org/dev/peps/pep-3126 REJECTED :( |
|
Thanks Jarek, I'm just grabbing a bite to eat |
|
Oh hmmm. I was kind of hoping it wouldn't catch cases like this tests/providers/google/cloud/hooks/test_cloud_sql.py:1207:0: W1404: Implicit string concatenation found in assignment (implicit-str-concat) uri = (
"gcpcloudsql://user:password@127.0.0.1:3200/testdb?database_type=mysql&"
"project_id=example-project&location=europe-west1&instance=testdb&"
"use_proxy=True&sql_proxy_use_tcp=True"
) |
|
I'm not sure this is worth it after all. |
|
What version of pylint do we use in breeze? Because locally I don't get nearly so many warnings. This is why |
|
It will break black formatting to -- donesn't it do this implicit str concat in cases. |
|
Fix here: astronomer#1110 |
It does not break black. Black at most moves stuff around but it will not remove '+' if it is there. |
Not sure. I stopped looking at versions once we have the common breeze env which is same for everyone, I just run it there :) |
|
Punting this to 2.1 |
…mmas-in-sequences Pyling check missing commas in sequences
Fine for me. |
|
I don't like this check. |
3 participants
Using implicit string concatenation in lists, even across multiple lines is almost always a sign of a bug, and Pylint has a check for this we can enable.
This should hopefully catch a few cases like #12880 from happening again.
There may be a few places in the code that his now starts warning about that we will have to fix as a result
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.