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

feat: make modules use black formatter #11

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

IgnacioHeredia
Copy link
Contributor

@vykozlov @BorjaEst

Maybe it's a good idea to start suggesting modules to use Black as formater, as it is starting to become a standard in many big Python projects.

Content of this PR:

  • modify tox style tests to work with Black
  • add some extra VScode goodies to make users life easier:
    • suggest installing Black Vscode extension
    • format code automatically on saves, so that there won't be any problems of style tests not passing

For reference, I recently implemented it in the demo app and it's working fine as far as I can tell.

@IgnacioHeredia IgnacioHeredia added the enhancement New feature or request label Aug 29, 2024
Copy link

sonarcloud bot commented Aug 29, 2024

@BorjaEst
Copy link
Contributor

Agree, 100%
BTW, is there a way to automate that? From experience a lot of people just do not do the autoformat so it is difficult that they will use it by themselves.
I saw some repos use github to run black during the PR merge process. Maybe there is something similar in Jenkins?

@IgnacioHeredia
Copy link
Contributor Author

For automating it, in the PR I have set the VSCode configuration to format on save then to save after delay (ie. save automatically). I've seen that it kind of works sometimes, but still need to save manually (CTRL + S) sometimes to format (or format directly with CTRL +SHIFT + I).

So yes, it could be nice to have something in Jenkins to auto-format, just in case.

@BorjaEst
Copy link
Contributor

BorjaEst commented Sep 2, 2024

I see it, nice add!
LGTM for the changes. I will copy them to the other templates and check.

@BorjaEst
Copy link
Contributor

BorjaEst commented Sep 5, 2024

tox checks for line length 80 but back is configured for 88

[testenv:qc.sty]
commands =
flake8 --statistics --tee --output-file={toxinidir}/flake8.log \
--max-line-length=80 --extend-select=B950 --extend-ignore=E203,E501,E701 \ # add this line to support Black formatter
--format=pylint {{ cookiecutter.__app_name }} tests

Is a reason why to check for 88?
I see more common options are 100 or 120.
Personally I like 120, but I do not mind any other value.

@BorjaEst
Copy link
Contributor

BorjaEst commented Sep 5, 2024

I would also recommend to add "isort".
Imports should also have some kind of structure and rules which I think black does not check.

{{ cookiecutter.__repo_name }}/README.md Show resolved Hide resolved
{{ cookiecutter.__repo_name }}/tox.ini Show resolved Hide resolved

For an optimal development experience, we recommend installing the VScode extensions
[Black](https://marketplace.visualstudio.com/items?itemName=ms-python.black-formatter)
and [Format on Auto Save](https://marketplace.visualstudio.com/items?itemName=BdSoftware.format-on-auto-save).
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if BdSoftware is needed?
I run it in my local and it was all working without it.
If we do not add BdSoftware then I would remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my case, formatting did not happen without BSsoftware extension.
I'm using VScode 1.92.1 on Ubuntu 20.04. As far as I can tell, people still complain it not working.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have 1.92.2 and U 22.04, however, It should be the same.
I was not trying exactly the same but rather the config here (updated):

https://github.com/ai4os-hub/demo-advanced/blob/main/.vscode/settings.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested with your configuration and it still does not work in my local. So to be on the save side, I would just keep the BSsoftware extension.

@IgnacioHeredia
Copy link
Contributor Author

tox checks for line length 80 but back is configured for 88

[testenv:qc.sty]
commands =
flake8 --statistics --tee --output-file={toxinidir}/flake8.log \
--max-line-length=80 --extend-select=B950 --extend-ignore=E203,E501,E701 \ # add this line to support Black formatter
--format=pylint {{ cookiecutter.__app_name }} tests

Is a reason why to check for 88? I see more common options are 100 or 120. Personally I like 120, but I do not mind any other value.

The tox configuration is the one recommended by Black. I think it's something like 80 + 10% --> 88

But I remember doing so when I manually installed black + bugbear with pip.
Now, the issue is the following. The VScode extension ships with the black binary. So I'm not sure it includes bugbear. Can you check?

Alternative options:

  1. installing black+bugbear as requirements and changing the VScode Black extension settings to use the local installed packages instead of the binary.
  2. using tox minimal configuration (I kind of prefer this one over nº1)

@IgnacioHeredia
Copy link
Contributor Author

I would also recommend to add "isort". Imports should also have some kind of structure and rules which I think black does not check.

I'm fine adding isort. Probably adding the Python extension (which includes isort) is a good idea, although this may add several other extensions that people may not want. So I'm fine adding only isort.

@BorjaEst
Copy link
Contributor

BorjaEst commented Sep 5, 2024

Bugbear does not ship with black.
Wee need to add it to requirements-test.txt:

flake8
bandit>=1.1.0 # Apache-2.0

We can fix to for example:

  • flake8 ~= 7.1.0
  • flake8-bugbear ~= 24.8.0

To do not use the binary, I tested adding the following:

    "black-formatter.path": [ "black" ],
    "black-formatter.args": [ "--line-length=80" ],

I never used "editor.rulers", but with --line-length=80 it should be in theory consistent with Bugbear.

You can check it is working by uninstalling black from the env and see it is not working.
Install again and check it works.

Sadly it does not raise a warning that you have to install black.

@IgnacioHeredia
Copy link
Contributor Author

Regarding the line-length, B950 sets the max-length to 88, though recommended length is still 80. For me it makes more sense the ruler to be at the max-length.

@IgnacioHeredia
Copy link
Contributor Author

    "black-formatter.path": [ "black" ],
    "black-formatter.args": [ "--line-length=80" ],

First line works, but as I said in the previous comment, I don't think we should touch Black default's line-length. It's already being taking care of by B950.

Sadly it does not raise a warning that you have to install black.

If we add black and bugbear in the requirements.txt (not the requirements-test.txt) we will be safer.

@BorjaEst
Copy link
Contributor

BorjaEst commented Sep 5, 2024

LGTM

@IgnacioHeredia
Copy link
Contributor Author

Sadly it does not raise a warning that you have to install black.

If we add black and bugbear in the requirements.txt (not the requirements-test.txt) we will be safer.

Well, in reality for formatting during development we don't need bugbear right? So we can use the Black binary (ie. do not change "black-formatter.path"). And flake8 and bugbear can be installed only in requirements-test.txt.

With this we avoid making the user install black package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants