fix: Fixed E731 errors in tests #737
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #737 +/- ##
=======================================
Coverage 96.96% 96.96%
=======================================
Files 20 20
Lines 1547 1547
=======================================
Hits 1500 1500
Misses 47 47 ☔ View full report in Codecov by Sentry. |
| @@ -102,7 +102,7 @@ def test_strip_comments_preserves_linebreak(self): | |||
| assert res == 'select *\n\nfrom foo' | |||
|
|
|||
| def test_strip_ws(self): | |||
| f = lambda sql: sqlparse.format(sql, strip_whitespace=True) | |||
| def f(sql): return sqlparse.format(sql, strip_whitespace=True) | |||
There was a problem hiding this comment.
ah, I've got a different opinion on this. E731 states to prefer a function definition over assigning a lambda expression to a variable. But as also stated in E731 the primary reason is for debugging as the lambda expression shows up as <lambda> in a traceback.
But here the lambda is just a shorthand for the formatting code to make the tests shorter and more readable. Iff debugging is needed here, there's only one <lambda>. So it's pretty obvious.
On the other hand I find single line function definitions a bit harder to read and I would avoid them if possible.
So to me "Readability counts" is the argument here, especially as the one reason for using function definitions instead of assigning a lambda function to a variable doesn't count here.
There was a problem hiding this comment.
Okay, I get you!)
I will close this PR now,
and thank you for your feedback
| @@ -140,8 +140,7 @@ def test_notransform_of_quoted_crlf(self): | |||
|
|
|||
| class TestFormatReindentAligned: | |||
| @staticmethod | |||
| def formatter(sql): | |||
| return sqlparse.format(sql, reindent_aligned=True) | |||
| def formatter(sql): return sqlparse.format(sql, reindent_aligned=True) | |||
There was a problem hiding this comment.
Why did you change this to a single line expression? It's harder to read, IMO.
| f = lambda sql: sqlparse.format(sql, output_format='python', | ||
| reindent=True) | ||
| def f(sql): return sqlparse.format(sql, output_format='python', reindent=True) | ||
| from pprint import pprint |
There was a problem hiding this comment.
Looks like you've left some debugging code in this change.
| assert f(sql) == "sql = 'select * from foo;'" | ||
| f = lambda sql: sqlparse.format(sql, output_format='python', | ||
| reindent=True) | ||
| def f(sql): return sqlparse.format(sql, output_format='python', reindent=True) |
There was a problem hiding this comment.
This and some of the following changes break the 80-column rule.
ghost
closed this
ghost
deleted the
Thanks for contributing!
Before submitting your pull request please have a look at the
following checklist:
pytest)flake8)In addition, please take care to provide a proper description
on what your change does, fixes or achieves when submitting the
pull request.