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

Add pre-commit hooks for code analysis and formatting #38

Merged
merged 19 commits into from
Feb 28, 2024

Conversation

MArpogaus
Copy link
Contributor

@MArpogaus MArpogaus commented Jan 29, 2024

Unfortunately i did initially fork from @oduerr, when i re-forked it from upstream this closed #35.
So i had to open this new PR.

Here a copy of the description from #35:

Hello @francois-rozet,

I hope you're doing well. I wanted to share my recent changes.
To make our codebase more consistent and maintainable, I've added some automated code formatters and static code analysis tools using the pre-commit framework.

(Edit: my first draft used a couple of linters (isort,flake8) and formatters (black, autoflake) in parallel. this adds unnecessary complexity, hence i decided to switch to a more modern approach using ruff)

Here's a quick overview of each hook and their repository links:

  • ruff An extremely fast Python linter and code formatter, written in Rust.

  • pre-commit-hooks: These hooks provide a range of useful features, including checking for trailing whitespace, validating YAML and TOML files, detecting accidentally added large files, and identifying merge conflicts.

  • language-formatters-pre-commit-hooks: The hooks in this repository enable pretty formatting of YAML and TOML files, making them more readable and following best practices.

By incorporating these automated code formatters and static code analysis tools into the project, we can save time during code reviews by minimizing manual fixes and ensuring that everyone follows consistent coding standards.

I'm really excited about these enhancements and how they'll benefit our collaboration.
Let's discuss these changes further! Let me know If you have any concerns or questions.

Best regards,
Marcel

.github/workflows/dummy.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
docs/tutorials/basics.ipynb Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
tests/test_distributions.py Outdated Show resolved Hide resolved
@MArpogaus
Copy link
Contributor Author

Hello @francois-rozet ,

thanks for your review, and sorry for dirty commit history. I did not have the time to clean it up on Monday,
I decided to completely revert onto current master and just re-add the relevant files with the suggested changes.

I did originally eliminate the use of all start import in commit 08d3f43.

Using star imports in Python is very convenient and save you from typing long module names repeatedly if you need to use multiple entities from a module.
However, from my experience star imports can cause naming conflicts if two modules have entities with the same name. Importing everything from a module can make it difficult to determine which specific entities are being used in the code. This can make code more error-prone and harder to debug. Additionally, Linters like ruff cant warn you about unused imports when start imports are used.

I am not dogmatic about the usage of star imports, and saw that you restricted them to be only used inside the tests and init modules, what seams to be common practice to me and could be considerd as a good balance between convenience and error-proofing. However, I would still suggest to remove them from the init modules for the reasons mentioned above, at the cost of slightly higher maintenance.

If desired i could cherry-pick zuko/flows/__init__.py from commit 08d3f43 and remove the ignores from pyprojetc.toml.
If not, i am also happy to merge the current state of this PR, if everything else is in line with your requirements.

@francois-rozet francois-rozet changed the title Add pre-commit hooks for static code analysis and code formattaing Add pre-commit hooks for code analysis and formatting Jan 31, 2024
Copy link
Member

@francois-rozet francois-rozet left a comment

Choose a reason for hiding this comment

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

I still need to go over the CONTRIBUTING.md edits. But here are already a few comments.

.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@francois-rozet
Copy link
Member

However, I would still suggest to remove them from the init modules for the reasons mentioned above, at the cost of slightly higher maintenance.

I am not against preventing star imports inside __init__.py files, but let's discuss that in another issue.

@MArpogaus
Copy link
Contributor Author

I am not against preventing star imports inside __init__.py files, but let's discuss that in another issue.

OK. Lets continue in #39.

@MArpogaus
Copy link
Contributor Author

MArpogaus commented Feb 13, 2024

I think I resolved all the remaining issues. Anything else that holds us back from closing this PR?

pre-commit.yml Outdated Show resolved Hide resolved
.github/workflows/push.yml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@francois-rozet francois-rozet left a comment

Choose a reason for hiding this comment

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

After these last fixes, I will test the feature and the workflow. If there is no issues, I'll merge!

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/workflows/push.yml Outdated Show resolved Hide resolved
.github/workflows/push.yml Outdated Show resolved Hide resolved
.github/workflows/push.yml Outdated Show resolved Hide resolved
.github/workflows/push.yml Outdated Show resolved Hide resolved
@francois-rozet
Copy link
Member

Hey @MArpogaus, sorry for the delay. Super busy this week... I'll try to review your PR(s) early next week. And thanks again for the great work!

Copy link
Member

@francois-rozet francois-rozet left a comment

Choose a reason for hiding this comment

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

Hello @MArpogaus, I tried the pre-commit hooks in the temp branch (I have to create a branch on the repo to test a workflow...). I had to make a few modifications, which you should also make in this PR.

However, I thought that the pre-commit workflow would prevent me to push code that is not consistent with our conventions, but it only runs checks.

.github/workflows/push.yml Outdated Show resolved Hide resolved
.github/workflows/push.yml Outdated Show resolved Hide resolved
pre-commit.yml Outdated Show resolved Hide resolved
@francois-rozet francois-rozet merged commit 1d2e52d into probabilists:master Feb 28, 2024
5 checks passed
@francois-rozet
Copy link
Member

I decided to merge the test, lint and pre-commit workflows in a single "continuous integration" workflow. It makes more sense like that. Thanks for the work, merged!

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.

3 participants