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

"make black" target doesn't actually fix something #13241

Closed
5 of 9 tasks
thedoubl3j opened this issue Nov 28, 2022 · 4 comments
Closed
5 of 9 tasks

"make black" target doesn't actually fix something #13241

thedoubl3j opened this issue Nov 28, 2022 · 4 comments
Labels
component:awx_collection issues related to the collection for controlling AWX component:docs type:bug

Comments

@thedoubl3j
Copy link
Member

Please confirm the following

  • I agree to follow this project's code of conduct.
  • I have checked the current issues for duplicates.
  • I understand that AWX is open source software provided for free and that I might not receive a timely response.

Bug Summary

the pre commit check for black checks to see if anything needs to be reformatted then says to make

AWX version

awx: 21.5.1.dev434+g3ca14c43bb

Select the relevant components

  • UI
  • API
  • Docs
  • Collection
  • CLI
  • Other

Installation method

N/A

Modifications

no

Ansible version

ansible [core 2.13.1]

Operating system

Fedora 36

Web browser

No response

Steps to reproduce

run make black at top level of the project with files that need to be reformatted.

Expected results

Files to be reformatted similar to targeting specific files with black.

Actual results

[thedoubl3j@localhost awx]$ git commit -m "commit msg"
would reformat awx_collection/plugins/lookup/schedule_rrule.py
would reformat awx_collection/plugins/lookup/schedule_rruleset.py

Oh no! 💥 💔 💥
2 files would be reformatted.
To fix this, run `make black` to auto-format your code prior to commit, or set AWX_IGNORE_BLACK=1
[thedoubl3j@localhost awx]$ make black
All done! ✨ 🍰 ✨
800 files left unchanged.

Additional information

current versions locally installed and which is being used

[thedoubl3j@localhost awx]$ black --version
black, 22.10.0 (compiled: yes)
Python (CPython) 3.9.15
[thedoubl3j@localhost awx]$ which black
~/.local/bin/black
@github-actions github-actions bot added component:awx_collection issues related to the collection for controlling AWX component:docs needs_triage type:bug labels Nov 28, 2022
@TheRealHaoLiu TheRealHaoLiu changed the title "make black" target doesn't actually fix anything "make black" target doesn't actually fix something Nov 28, 2022
@relrod
Copy link
Member

relrod commented Nov 28, 2022

I think this is because the pre-commit check runs black on against all changed files, without respect to files that black is told to ignore:

awx/pre-commit.sh

Lines 2 to 9 in fcdff8b

python_files_changed=$(git diff --cached --name-only --diff-filter=AM | grep -E '\.py')
if [ "x$python_files_changed" != "x" ] ; then
black --check $python_files_changed || \
if [ $? != 0 ] ; then
echo 'To fix this, run `make black` to auto-format your code prior to commit, or set AWX_IGNORE_BLACK=1'
exit 1
fi
fi

And then we configure black to ignore awx_collection:

exclude = "awx_collection"

@thedoubl3j
Copy link
Member Author

Is there any reason anyone could think of that we do not extend black to the collection?

@AlanCoding
Copy link
Member

Yeah, we would be battling between black and pep8 and other things that ansible-test runs. I don't think it would be viable.

@AlanCoding
Copy link
Member

Resolved with linked merge (hopefully)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:awx_collection issues related to the collection for controlling AWX component:docs type:bug
Projects
None yet
Development

No branches or pull requests

4 participants