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

Update black rev in order to include fix for https://github.com/psf/black/issues/1629 #92

Closed
wants to merge 1 commit into from

Conversation

tafaRU
Copy link
Member

@tafaRU tafaRU commented Nov 23, 2021

I ran into this error related to psf/black#1629.
In order to fix it, black needs to be updated at least to https://pypi.org/project/black/21.4b0.

tafaRU added a commit to tafaRU/l10n-italy that referenced this pull request Nov 23, 2021
tafaRU added a commit to tafaRU/l10n-italy that referenced this pull request Nov 23, 2021
@@ -1,6 +1,6 @@
repos:
- repo: https://github.com/psf/black
rev: 19.10b0
rev: 21.4b0
Copy link
Contributor

@simahawk simahawk Nov 23, 2021

Choose a reason for hiding this comment

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

this is the repo's config. I guess you want to change this instead https://github.com/OCA/oca-addons-repo-template/blob/master/src/.pre-commit-config.yaml.jinja#L4 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thanks for the advice @simahawk!

@@ -3,7 +3,7 @@

{%- if odoo_version < 15 %}
{%- set repo_rev.autoflake = "v1.4" %}
{%- set repo_rev.black = "20.8b1" %}
{%- set repo_rev.black = "21.4b0" %}
Copy link
Member

Choose a reason for hiding this comment

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

This can be a dangerous change in branches bellow v15, as it can introduce some code formatting changes. We did hold back on bumping the version before for this reason, so maybe @sbidoul and @yajo should comment on this and say if they are OK with it.

tafaRU added a commit to tafaRU/l10n-italy that referenced this pull request Nov 23, 2021
@sbidoul
Copy link
Member

sbidoul commented Nov 23, 2021

The problem with changing black on active branches is that it creates some churn with PRs becoming red due to minor reformating. That said, black is getting more and more stable over time.

Alternatives:

  • using the # fmt: on/off/skip black directive to work around local issues
  • making the black version a copier question (with same defaults as today), so we can let PSCs manage their black version autonomously across branches

@tafaRU
Copy link
Member Author

tafaRU commented Nov 23, 2021

Ok, thanks for the explanation.
In the meantime I found the lines could make black fail and I fix them. So I can proceed without this.

Personally I'd prefer the following:

making the black version a copier question (with same defaults as today), so we can let PSCs manage their black version autonomously across branches

@tafaRU tafaRU closed this Nov 23, 2021
@tafaRU tafaRU deleted the fix-black-issue1629 branch November 23, 2021 16:27
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.

None yet

4 participants