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

chore: fix pre-commit failures #22

Merged

Conversation

m4tx
Copy link
Contributor

@m4tx m4tx commented May 13, 2022

Pre-commit hooks used to fail on the project's master branch when
running on all files; this commit fixes that.

@vladfedoriuk
Copy link
Collaborator

In general, we try to avoid reformatting the migrations files. Presumably, you ran pre-commit run --all-files, which in turn executed black <path of every file in the project> under the hood, so black basically disregarded the configurations from pyproject.toml. One way to fix it is providing --force-exclude options as done in here. Please, do it in this PR. What about reformatted migrations, I do not have anything against those particular changes, as those files seem to have already been reformatted once, but in the future, we would like to omit such diffs.

@ConsoleRunner
Copy link
Collaborator

ConsoleRunner commented May 13, 2022

What exactly did it fix? It seems that the PR only contains re-formatting changes, none of which are connected to the pre-commit config (you might be missing the config file changes). Additionally, while running all formatters in root might look like a good idea, we intend to place frontend (non-python) files in a /frontend file beside /backend, I'm afraid that it will cause problems with some autoformatters down the road.

Moreover, it seems that pyproject.toml was omitted (migrations were not excluded from being reformatted). Omitting some files from autoformatting was our decision, and we're happy to discuss it in more detail, but it shows that the respective config was ignored (possibly due to --all-files flag).

@m4tx m4tx force-pushed the pre-commit-fix branch from f0aeeb6 to 74e4088 Compare May 13, 2022 13:40
@m4tx
Copy link
Contributor Author

m4tx commented May 13, 2022

In general, we try to avoid reformatting the migrations files. Presumably, you ran pre-commit run --all-files, which in turn executed black <path of every file in the project> under the hood, so black basically disregarded the configurations from pyproject.toml. One way to fix it is providing --force-exclude options as done in here. Please, do it in this PR.

No, it doesn't solve the issue at all. You don't have to run pre-commit run -a to observe this issue - you can just add an empty comment in backend/fregepoc/indexers/migrations/0004_add_sourceforgeindexer.py (or just any other file I've modified in this PR) to either face auto-reformatting or linter issues. If you don't want black to run on certain files, exclude them in pre-commit config; not in pyproject.toml (which is ignored by pre-commit command, and therefore, hooks anyway).

What about reformatted migrations, I do not have anything against those particular changes, as those files seem to have already been reformatted once, but in the future, we would like to omit such diffs.

Again, exclude them from being automatically reformatted in pre-commit hooks.

What exactly did it fix? It seems that the PR only contains re-formatting changes, none of which are connected to the pre-commit config (you might be missing the config file changes).

I haven't changed the pre-commit config - and that's the whole point of this PR. I've indeed run pre-commit run -a, and I expected everything to be okay - but it wasn't.

Additionally, while running all formatters in root might look like a good idea, we intend to place frontend (non-python) files in a /frontend file beside /backend, which may cause problems with some autoformatters down the road.

It will happen unless you exclude them from being run in .pre-commit-config.yaml.

Moreover, it seems that pyproject.toml was omitted (migrations were not excluded from being reformatted). Omitting some files from autoformatting was our decision, and we're happy to discuss it in more detail, but it shows that the respective config was ignored (possibly due to --all-files flag).

I guess my comments above answer that as well.

@apardyl
Copy link
Contributor

apardyl commented May 13, 2022

+1 to what @m4tx reported, pre-commit is broken.

@m4tx
Copy link
Contributor Author

m4tx commented May 13, 2022

Anyway, it wasn't really the point of this PR, but I've modified the pre-commit config. Next time, please make sure that the files in your repository conform to the pre-commit hooks before you push them forcefully to everyone.

@m4tx m4tx force-pushed the pre-commit-fix branch from 74e4088 to 58edd28 Compare May 13, 2022 13:44
@vladfedoriuk
Copy link
Collaborator

Anyway, it wasn't really the point of this PR, but I've modified the pre-commit config. Next time, please make sure that the files in your repository conform to the pre-commit hooks before you push them forcefully to everyone.

Thanks a lot for a kind reminder, though I do not quite understand the push them forcefully part as pre-commit config was merged in the same way as any other branch.

@m4tx m4tx force-pushed the pre-commit-fix branch from 58edd28 to 1adf5d4 Compare May 13, 2022 14:06
@m4tx
Copy link
Contributor Author

m4tx commented May 13, 2022

Anyway, it wasn't really the point of this PR, but I've modified the pre-commit config. Next time, please make sure that the files in your repository conform to the pre-commit hooks before you push them forcefully to everyone.

Thanks a lot for a kind reminder, though I do not quite understand the push them forcefully part as pre-commit config was merged in the same way as any other branch.

What I've meant is that, because .pre-commit-config.yaml exists in the repository, all of the contributors are supposed to use it. It's kind of unfortunate for them to run into linter issues when modifying some files on the repo just because nobody thought to run the checks on all files before pushing the config. Note that pre-commit runs the checks on the entire file that was modified, not just the modified lines - so it's not too difficult to eventually face the problems because of that.

Copy link
Contributor

@apardyl apardyl left a comment

Choose a reason for hiding this comment

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

LGTM, pls rebase

Copy link
Collaborator

@ConsoleRunner ConsoleRunner left a comment

Choose a reason for hiding this comment

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

Rebase requested

Pre-commit hooks used to fail on the project's master branch when
running on all files; this commit fixes that.
@m4tx m4tx force-pushed the pre-commit-fix branch from 1adf5d4 to ec90d88 Compare May 24, 2022 18:38
@m4tx
Copy link
Contributor Author

m4tx commented May 24, 2022

@ConsoleRunner done

@ConsoleRunner ConsoleRunner merged commit a5b5465 into Software-Engineering-Jagiellonian:master May 24, 2022
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.

5 participants