RedHat-Israel / ROSE

ROSE project car race game
GNU General Public License v2.0
34 stars 121 forks source link

Add black lint #483

Closed kiblik1 closed 10 months ago

kiblik1 commented 10 months ago

fixes #482 The apply black codestyle is huge I know :( but I don´t know any way to disable the job for old files and it would have to be run eventually.

yaacov commented 10 months ago

nitpick II:

we should add black to the requirements file: https://github.com/RedHat-Israel/ROSE/blob/master/requirements-dev.txt

not sure if use this files: https://github.com/RedHat-Israel/ROSE/blob/master/Pipfile - do we need to keep it? if we do, wee need to add black https://github.com/RedHat-Israel/ROSE/blob/master/.travis.yml - anyone run it ? if we do, wee need to add black

sleviim commented 10 months ago

lint failed: ./docs/course_materials/exercises/test_exercises/03_Variables_and_datatypes/test_homework_strings.py:62:17: W503 line break before binary operator

nirs commented 10 months ago

lint failed: ./docs/course_materials/exercises/test_exercises/03_Variables_and_datatypes/test_homework_strings.py:62:17: W503 line break before binary operator + "make sure to include the previous variables", ^ 16 W503 line break before binary operator Error: Process completed with exit code 1.

This means the code needs reformatting with black. @kiblik1 can you do reformat and post a version that pass CI?

yaacov commented 10 months ago

@nirs W503 is very strange rule, pep thinks it's ok to break lines before operator: https://peps.python.org/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

W503 should be disabled by default ? maybe best is to add it to the ignore rules ?

Ref: https://black.readthedocs.io/en/stable/guides/using_black_with_other_tools.html#id1

When breaking a line, Black will break it before a binary operator. This is compliant with PEP 8 as of April 2016. There’s a disabled-by-default warning in Flake8 which goes against this PEP 8 recommendation called W503 line break before binary operator. It should not be enabled in your configuration.

p.s. @kiblik1 hi, sorry for comment https://github.com/RedHat-Israel/ROSE/pull/483#discussion_r1308464145 I see you folowed the rules in blake docs If you change the rules and add W503, you should revert and use black recommendations, like you originally did, @nirs WDYT ?

nirs commented 10 months ago

@yaacov right, disable W503.

sleviim commented 10 months ago

Hi, not sure who should disable it. @yaacov is it you? (sorry if it's not)

yaacov commented 10 months ago

@yaacov is it you?

@sleviim I don't have permissions, it's you, @nir or @kiblik1

sleviim commented 10 months ago

Wondering if it's better to squash all those changes or merge them as is @nirs @yaacov

yaacov commented 10 months ago

yes, I am for squashing :squid: