Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add black lint #483

Merged
merged 10 commits into from
Sep 12, 2023
Merged

Add black lint #483

merged 10 commits into from
Sep 12, 2023

Conversation

kiblik1
Copy link
Contributor

@kiblik1 kiblik1 commented Aug 28, 2023

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.

@nirs nirs requested review from yaacov and nirs August 29, 2023 09:07
.flake8 Outdated Show resolved Hide resolved
Comment on lines 22 to 25
reformat:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this action do ?
will it fail if the code is not formatted correctly ?

if it checks for changes after applying black then it's actually a linter ?
and if it just apply changes (e.g. reforamt) maybe we don't need it as ci ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this action will fail on bad formating, so it's actually a linter action,
can we rename it "blake" , because we already do a basic linting using flake8

yaacov

This comment was marked as outdated.

Comment on lines 22 to 26
reformat:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: psf/black@stable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that the --check flag is required so that the workflow fails if Black finds files that need to be formatted.
options: "--check --verbose"

If we want to fail code contributions that do not follow code standard we need to add
Ref: https://black.readthedocs.io/en/stable/integrations/github_actions.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see that the --check is default, no need to add it ... 👍

@yaacov
Copy link
Member

yaacov commented Aug 29, 2023

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

yaacov

This comment was marked as outdated.

Use better formatting of necessary changes to comply with Black codestyle

Co-authored-by: Yaacov Zamir <kobi.zamir@gmail.com>
Copy link
Member

@yaacov yaacov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blint is too creative but we can improve it later.

@sleviim can you ack the changes in the course python code?

Copy link
Member

@sleviim sleviim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOW!
I definitely like this one!

Copy link
Member

@sleviim sleviim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions, please share your thoughts.
It looks really good without my suggestions as well, so I can merge it anyway if you like.

rose/client/main.py Show resolved Hide resolved
rose/common/actions.py Show resolved Hide resolved
rose/common/obstacles.py Show resolved Hide resolved
rose/server/score_test.py Show resolved Hide resolved
@sleviim
Copy link
Member

sleviim commented Sep 5, 2023

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.

@nirs
Copy link
Member

nirs commented Sep 5, 2023

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
Copy link
Member

yaacov commented Sep 6, 2023

@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 #483 (comment) 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
Copy link
Member

nirs commented Sep 6, 2023

@yaacov right, disable W503.

@sleviim
Copy link
Member

sleviim commented Sep 11, 2023

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

@yaacov
Copy link
Member

yaacov commented Sep 11, 2023

@yaacov is it you?

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

@sleviim
Copy link
Member

sleviim commented Sep 12, 2023

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

@yaacov
Copy link
Member

yaacov commented Sep 12, 2023

yes, I am for squashing 🦑

@sleviim sleviim merged commit 4c9b4aa into RedHat-Israel:master Sep 12, 2023
1 check passed
dolev313 pushed a commit to dolev313/ROSE that referenced this pull request Jul 21, 2024
* Fix RedHat-Israel#482

* apply black codestyle

* modify flake8 settings to be compatible with Black

* change job name to be less ambiguous

* integrate black into ci as a step reather than a standalone job and add it to the dev deps

* Update .flake8

Use better formatting of necessary changes to comply with Black codestyle

Co-authored-by: Yaacov Zamir <kobi.zamir@gmail.com>

* add W508 to ignored rules

* fix typo

* fix yet another typo...

---------

Co-authored-by: Yaacov Zamir <kobi.zamir@gmail.com>
dolev313 added a commit to dolev313/ROSE that referenced this pull request Jul 25, 2024
* Fix RedHat-Israel#482

* apply black codestyle

* modify flake8 settings to be compatible with Black

* change job name to be less ambiguous

* integrate black into ci as a step reather than a standalone job and add it to the dev deps

* Update .flake8

Use better formatting of necessary changes to comply with Black codestyle

Co-authored-by: Yaacov Zamir <kobi.zamir@gmail.com>

* add W508 to ignored rules

* fix typo

* fix yet another typo...

---------

Co-authored-by: Yaacov Zamir <kobi.zamir@gmail.com>
dolev313 added a commit to dolev313/ROSE that referenced this pull request Jul 25, 2024
* Fix RedHat-Israel#482

* apply black codestyle

* modify flake8 settings to be compatible with Black

* change job name to be less ambiguous

* integrate black into ci as a step reather than a standalone job and add it to the dev deps

* Update .flake8

Use better formatting of necessary changes to comply with Black codestyle

Co-authored-by: Yaacov Zamir <kobi.zamir@gmail.com>

* add W508 to ignored rules

* fix typo

* fix yet another typo...

---------

Co-authored-by: Yaacov Zamir <kobi.zamir@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use black for automatic code formatting
4 participants