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 lint-fix doesn't fix linting issue #1036

Closed
andyndang opened this issue Dec 23, 2022 · 3 comments · Fixed by #1057
Closed

make lint-fix doesn't fix linting issue #1036

andyndang opened this issue Dec 23, 2022 · 3 comments · Fixed by #1057

Comments

@andyndang
Copy link
Contributor

Description

However the CI is faiilng

- files were modified by this hook

reformatted python/tests/core/view/test_dataset_profile.py

All done! ✨ 🍰 ✨

@jamie256
Copy link
Contributor

make pre-commit will fix these up, we can pipe the lint/format fix through that.

@jamie256
Copy link
Contributor

Ok, I just reproduced this issue even with running pre-commit, I don't see the issue yet but see that it is an inconsistent result in running black.

@jamie256
Copy link
Contributor

jamie256 commented Jan 19, 2023

poetry run black --check . fails while at the same time poetry run pre-commit run --all-files runs and passes black:

[INFO] Running pre-commit checks

poetry run pre-commit run --all-files
black....................................................................Passed
flake8...................................................................Passed
isort....................................................................Passed
mypy.....................................................................Passed
prettier.................................................................Passed
check for added large files..............................................Passed
debug statements (python)................................................Passed
detect aws credentials...................................................Passed
detect private key.......................................................Passed
fix end of files.........................................................Passed
forbid new submodules................................(no files to check)Skipped
mixed line ending........................................................Passed
trim trailing whitespace.................................................Passed

And then poetry run black --check .

poetry run black . --check
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install "black[jupyter]"``
would reformat whylogs/api/writer/whylabs.py

Oh no! 💥 💔 💥
1 file would be reformatted, 139 files would be left unchanged.

This happens because there is a different version of black in the poetry.lock file: 23.1a1 vs in the .pre-commit-config.yaml which has 22.3.0, and they disagree on if an empty line as first line in a function body is acceptable or not.

Will fix this by making the version of black consistent between pre-commit and poetry.

@jamie256 jamie256 mentioned this issue Jan 19, 2023
1 task
jamie256 added a commit that referenced this issue Jan 19, 2023
We have both make format and pre-commit, but the pre-commit checks
perform a larger scope of checks and used different versions of some of
the tools (such as black) in the .pre-commit-config.yaml file vs
poetry.lock file leading to inconsistent lint and formatting checks.

whylogs will deprecate the `make format` target in favor of `make
pre-commit`, and align the versions of the hooks used with those
installed in developer environments via poetry.

## Changes

- Deprecate make format and make format fix with warning and call make
pre-commit for both of these.
- Add pre-commit as a precondition of make release (replacing make
format and lint).
- Remove redundant format and lint step in the ci since these are run
again as part of make release
- update version of black in .pre-commit-config.yaml to match what we
run in poetry environment.
- formatting fixes so the repo complies with updated black and ci.

## Related

fixes #1036
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 a pull request may close this issue.

2 participants