Skip to content

refactor: pre-commit triggered from tox #2320

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 20, 2022
Merged

refactor: pre-commit triggered from tox #2320

merged 1 commit into from
Oct 20, 2022

Conversation

lmilbaum
Copy link

@lmilbaum lmilbaum commented Oct 17, 2022 •
edited
Loading

The project is using two liniting frameworks: tox and pre-commit. Each has its own configuration files and a CI workflow.
The purpose of this PR is to consolidate linting into one framework: tox
Discussion - #2321

This PR is a baby step towards a full transition

@JohnVillalovos
Copy link
Member

I personally don't like this as I do NOT use pre-commit. I like to do everything in tox. And I do not want to use pre-commit.

@lmilbaum lmilbaum force-pushed the refactoring branch 3 times, most recently from b6f5bf3 to dd7730e Compare October 17, 2022 05:08
@lmilbaum
Copy link
Author

I personally don't like this as I do NOT use pre-commit. I like to do everything in tox. And I do not want to use pre-commit.

Good point. I should have raised this topic in a discussion.

@lmilbaum lmilbaum force-pushed the refactoring branch 9 times, most recently from 29cafad to ca5b6a7 Compare October 19, 2022 10:57
@lmilbaum lmilbaum changed the title refactor: lint duplications in tox and pre-commit refactor: pre-commit trigger from tox Oct 19, 2022
nejch
nejch reviewed Oct 19, 2022
View reviewed changes
@lmilbaum lmilbaum force-pushed the refactoring branch 2 times, most recently from 0042e7a to 214c2d7 Compare October 19, 2022 11:12
@lmilbaum lmilbaum requested a review from nejch October 19, 2022 11:16
@lmilbaum lmilbaum marked this pull request as ready for review October 19, 2022 11:31
@lmilbaum lmilbaum changed the title refactor: pre-commit trigger from tox refactor: pre-commit triggered from tox Oct 19, 2022
nejch
nejch reviewed Oct 19, 2022
View reviewed changes
@lmilbaum lmilbaum mentioned this pull request Oct 19, 2022
Merged
@lmilbaum lmilbaum marked this pull request as draft October 19, 2022 16:53
@lmilbaum lmilbaum marked this pull request as ready for review October 19, 2022 18:06
@lmilbaum lmilbaum requested a review from nejch October 19, 2022 18:06
JohnVillalovos
JohnVillalovos approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@JohnVillalovos JohnVillalovos left a comment

Choose a reason for hiding this comment

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

This works for me. As I usually run a subset of the tox checks in my development process.

I like that I don't have to run pre-commit and can just use tox 🙂

So I approve but let @nejch do the merging if he approves.

@nejch
Copy link
Member

nejch commented Oct 20, 2022

time tox gives me 50s versus 20s before so running the default env list takes a bit long now, but let's get this in and work on that later :)

@nejch nejch merged commit eec6c02 into python-gitlab:main Oct 20, 2022
@lmilbaum lmilbaum deleted the refactoring branch October 20, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnVillalovos JohnVillalovos JohnVillalovos approved these changes

@nejch nejch Awaiting requested review from nejch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lmilbaum @JohnVillalovos @nejch