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 pre-commit #222

Merged
merged 1 commit into from
Mar 30, 2022
Merged

Update pre-commit #222

merged 1 commit into from
Mar 30, 2022

Conversation

Uxio0
Copy link
Member

@Uxio0 Uxio0 commented Mar 30, 2022

No description provided.

@Uxio0 Uxio0 requested review from a team, rmeissner, jpalvarezl and luarx March 30, 2022 11:07
@coveralls
Copy link

coveralls commented Mar 30, 2022

Coverage Status

Coverage decreased (-0.3%) to 90.557% when pulling 8d17028 on update-precommit into e8959c6 on master.

Comment on lines 8 to 11
- repo: https://github.com/psf/black
rev: 22.1.0
rev: 22.3.0
hooks:
- id: black
Copy link
Contributor

@fmrsabino fmrsabino Mar 30, 2022

Choose a reason for hiding this comment

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

We have this pre-commit hook but it seems that we don't declare black as a dependency.

Should we provide this in requirements-dev.txt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pre-commit installs its packages independently

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes under a pre-commit environment. It's still not available for in the normal venv right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No

Copy link
Contributor

Choose a reason for hiding this comment

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

No

Ok so we don't add black to the dependencies because it's under the pre-commit.
I would say that we can remove flake8 and isort from requirements-dev.txt so that we don't have any conflict with the versions.

Copy link
Member Author

@Uxio0 Uxio0 Mar 30, 2022

Choose a reason for hiding this comment

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

I think we can remove isort, but flake8 is used by the editor (like vim or vscode). Thinking about it, I think we can add black to get auto formatting in the IDE and not remove anything, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove isort, but flake8 is used by the editor (like vim or vscode). Thinking about it, I think we can add black to get auto formatting in the IDE and not remove anything, what do you think?

I think that's fine 👍

Was just wondering about the value of having those in requirements-dev.txt and also in the pre-commit. In the safe-config-service we have on both (the disadvantage is that the versions need to be kept in sync) 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand. IMHO is just convenience

@Uxio0 Uxio0 merged commit 92da164 into master Mar 30, 2022
@Uxio0 Uxio0 deleted the update-precommit branch March 30, 2022 12:21
@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants