Create lint_python.yml #3527
Create lint_python.yml #3527
Conversation
|
It's good that CI runs this automatically. The run fails and those failures should obviously be fixed before this PR could be accepted. On a quick look things that should be done are:
|
.github/workflows/lint_python.yml
Outdated
| - run: flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics | ||
| - run: | | ||
| EXCLUDE=./.*,./atest/testdata/* | ||
| flake8 . --count --exclude=$EXCLUDE --select=E9,F63,F7,F82 --show-source --statistics |
There was a problem hiding this comment.
Could this just be run for src/robot instead of of trying to exclude other directories?
There was a problem hiding this comment.
Do we just ignore problems that appear in setup.py for instance? I think it is safer to only exclude certain test directories where you are consciously trying to break things. Also, should we just paper over the fact that this code does not follow the Python porting best practice use feature detection instead of version detection?
There was a problem hiding this comment.
-
If you think it's important to test
setup.py, run linter for it in addition tosrc/robot. I want linting to be opt-in, not opt-out, in general and I definitely don't want it for any code under atest or utest directories. -
This code base uses both version detection and feature detection, depending on which is more practical. Most of the Python 2/3 differences are such that version detection is 100% safe, results with cleaner code, and is also a bit faster (
try/exceptisn't free). OurPY3constant is defined so that it will be true also with possible Python 4, so as long it doesn't break compatibility with Python 3 we are safe. And if it breaks, we need to fix our code anyway. Finally, this stuff is definitely not going to change before RF 4.0 when we remove Python 2 support altogether.
Codecov Report
@@ Coverage Diff @@
## master #3527 +/- ##
==========================================
- Coverage 74.12% 72.41% -1.70%
==========================================
Files 199 197 -2
Lines 16452 16027 -425
Branches 2646 2636 -10
==========================================
- Hits 12194 11605 -589
- Misses 3823 3955 +132
- Partials 435 467 +32
Continue to review full report at Codecov.
|
As requested at robotframework#3313 (comment) This is a exact replica of robotframework#3421
| @@ -125,12 +125,13 @@ def unknown(argument: Unknown, expected=None): | |||
| _validate_type(argument, expected) | |||
|
|
|||
|
|
|||
| def non_type(argument: 'this is string, not type', expected=None): | |||
| def non_type(argument: 'this is string, not type', # noqa: F821 | |||
| expected=None): | |||
There was a problem hiding this comment.
I don't want any validation in atest and utest directories. There's lot of strange code for testing purposes and thus lot of of linting errors. I don't want them to be excluded from linting one by one.
| @@ -13,7 +13,7 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| from robot.utils import PY2 | |||
| from robot.utils.robottypes import long, unicode | |||
There was a problem hiding this comment.
Our code style is importing everything from second level packages like robot.utils when the importing code is outside of that package. That makes it possible to change the internal package structure without breaking anything externally and listing everything that is exposed in __init__.py also makes the API better.
In other words, this import should be changed to form robot.utils import long, unicode. long probably should be explicitly exposed via robot.utils as well.
| @@ -17,6 +17,7 @@ | |||
| from collections import OrderedDict | |||
|
|
|||
| from robot.utils import IRONPYTHON, PY_VERSION | |||
| from robot.utils.robottypes import long | |||
| @@ -45,7 +45,7 @@ | |||
| html_escape, html_format, IRONPYTHON, is_string, | |||
| PY_VERSION, secs_to_timestr, seq2str2, | |||
| timestr_to_secs, unescape) | |||
|
|
|||
| from robot.utils.robottypes import long | |||
There was a problem hiding this comment.
Add long to the above from robot.utils import ... list.
The removed line would also be a PEP-8 violation as there's now only one empty line after imports.
| @@ -16,6 +16,7 @@ | |||
| import sys | |||
|
|
|||
| from .platform import IRONPYTHON, PY2 | |||
| from .robottypes import unicode | |||
There was a problem hiding this comment.
In this module unicode is used only in a Python 2 only block. Perhaps using the #noqa comment would be better in this case.
| def is_unicode(item): | ||
| return isinstance(item, unicode) | ||
|
|
||
|
|
There was a problem hiding this comment.
Please revert this change. I like having PY2 and PY3 code in separate modules in this case. Also, this PR should be only about linting.
| assert_equal(unicode(nd), "{'a': u'\\xe4', u'\\xe4': 'a'}") | ||
| else: | ||
| except NameError: |
There was a problem hiding this comment.
NO! We definitely absolutely never ever want to have try/except in test code like this! It can too easily cause false positives. Also, if PY2 makes it a lot more clear why we have two different assertions.
The whole utest directory should be excluded from linting altogether.
|
I got here to mention that I simplified |
As requested at #3313 (comment) This is a exact replica of #3421