refactor: Re-enable pylint on 5 files #10106
Conversation
superset/connectors/sqla/models.py
Outdated
| ) # pylint: disable=attribute-defined-outside-init | ||
| dbcol.avg = ( | ||
| dbcol.is_numeric | ||
| ) # pylint: disable=attribute-defined-outside-init |
There was a problem hiding this comment.
OK, what's the deal with these two attributes? Is this cruft? Can I kill it? does anyone know?
There was a problem hiding this comment.
Yay pylint paying dividends. I think these could be annexed as they don't seem to be part of the TableColumn class nor seem to be used.
There was a problem hiding this comment.
I think that the comment applies to the block, meaning you can:
# pylint: disable=attribute-defined-outside-init
dbcol.sum = dbcol.is_numeric
dbcol.avg = dbcol.is_numerichttps://docs.pylint.org/en/1.6.0/faq.html#is-it-possible-to-locally-disable-a-particular-message
It'd be nice to have more scoping options in pylint, but block is a good place to start.
| @@ -924,9 +934,15 @@ def get_sqla_query( # sqla | |||
| elif op == utils.FilterOperator.LIKE.value: | |||
| where_clause_and.append(col_obj.get_sqla_col().like(eq)) | |||
| elif op == utils.FilterOperator.IS_NULL.value: | |||
| where_clause_and.append(col_obj.get_sqla_col() == None) | |||
| where_clause_and.append( | |||
| col_obj.get_sqla_col() # pylint: disable=singleton-comparison | |||
There was a problem hiding this comment.
I think is None and is not None will resolve lines 938 and 943 respectively.
There was a problem hiding this comment.
You know, I thought about that, even tried it, and I'm pretty sure that'll break the function:
>>> from sqlalchemy.sql import column
>>> column("test") is not None
True
>>> column("test") is None
False
>>> column("test") == None
<sqlalchemy.sql.elements.BinaryExpression object at 0x108445e48>
>>> column("test") != None
<sqlalchemy.sql.elements.BinaryExpression object at 0x108450668>I believe this is building up a where clause, not returning booleans.
superset/connectors/sqla/models.py
Outdated
| ) # pylint: disable=attribute-defined-outside-init | ||
| dbcol.avg = ( | ||
| dbcol.is_numeric | ||
| ) # pylint: disable=attribute-defined-outside-init |
There was a problem hiding this comment.
Yay pylint paying dividends. I think these could be annexed as they don't seem to be part of the TableColumn class nor seem to be used.
Codecov Report
@@ Coverage Diff @@
## master #10106 +/- ##
==========================================
+ Coverage 65.79% 70.89% +5.09%
==========================================
Files 590 400 -190
Lines 31152 12909 -18243
Branches 3169 3174 +5
==========================================
- Hits 20497 9152 -11345
+ Misses 10477 3641 -6836
+ Partials 178 116 -62
Continue to review full report at Codecov.
|
willbarrett
force-pushed
the
wbarrett/lint-it
branch
from
271e34c to
8bca4fd
Compare
willbarrett
force-pushed
the
wbarrett/lint-it
branch
from
ee2b8e3 to
848727f
Compare
|
I think I've addressed comments - ready for another pass. |
* Re-enable lint on 5 files * revert something questionable * Address PR feedback * One more PR comment... * black? * Update code wrapping * Disable bugged check * Add a disable for a failure that's only showing up in CI. * Fix bad refactor * A little more lint fixing, bug fixing
mistercrunch
added
the
🏷️ bot
SUMMARY
Re-enables pylint on 5 files with global disables. Some checks are fixed, others disabled on a per-instance basis.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
CI, PR review
ADDITIONAL INFORMATION
Reviewers
@john-bodley @craig-rueda @dpgaspar