Skip to content

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

Merged
francois-rozet merged 19 commits intoprobabilists:masterfrom
MArpogaus:pre-commit
Feb 28, 2024
Merged

Add pre-commit hooks for code analysis and formatting#38
francois-rozet merged 19 commits intoprobabilists:masterfrom
MArpogaus:pre-commit

Conversation

@MArpogaus
Copy link
Copy Markdown
Contributor

@MArpogaus MArpogaus commented Jan 29, 2024 •
edited
Loading

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

francois-rozet
francois-rozet reviewed Jan 29, 2024
View reviewed changes
@MArpogaus
Copy link
Copy Markdown
Contributor Author

MArpogaus commented Jan 31, 2024

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
francois-rozet
francois-rozet reviewed Jan 31, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@francois-rozet
Copy link
Copy Markdown
Member

francois-rozet commented Jan 31, 2024

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 MArpogaus mentioned this pull request Feb 1, 2024
Closed
@MArpogaus
Copy link
Copy Markdown
Contributor Author

MArpogaus commented Feb 1, 2024

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
Copy Markdown
Contributor Author

MArpogaus commented Feb 13, 2024 •
edited
Loading

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

francois-rozet
francois-rozet reviewed Feb 14, 2024
View reviewed changes
francois-rozet
francois-rozet reviewed Feb 15, 2024
View reviewed changes
Copy link
Copy Markdown
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!

MArpogaus and others added 4 commits February 16, 2024 20:08
@MArpogaus @francois-rozet
add link to explain pip -e
c8f8edb 
Co-authored-by: François Rozet <francois.rozet@outlook.com>
@MArpogaus @francois-rozet
bump pre-commit/action to v3.0.1
53765ec 
Co-authored-by: François Rozet <francois.rozet@outlook.com>
@MArpogaus
remove pre-commit section from contribution guidelines
a0f3855
@MArpogaus @francois-rozet
add link to pre-commit.yml
a2c783a 
Co-authored-by: François Rozet <francois.rozet@outlook.com>
ulgens
ulgens suggested changes Feb 19, 2024
View reviewed changes
ulgens
ulgens reviewed Feb 21, 2024
View reviewed changes
@MArpogaus @ulgens
pin only major version of pre-commit action
ded5082 
Co-authored-by: Ülgen Sarıkavak <ulgens@users.noreply.github.com>
@francois-rozet
Copy link
Copy Markdown
Member

francois-rozet commented Feb 22, 2024

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!

francois-rozet
francois-rozet reviewed Feb 27, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@francois-rozet francois-rozet left a comment •
edited
Loading

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.

francois-rozet
francois-rozet reviewed Feb 27, 2024
View reviewed changes
@francois-rozet francois-rozet merged commit 1d2e52d into probabilists:master Feb 28, 2024
@francois-rozet
Copy link
Copy Markdown
Member

francois-rozet commented Feb 28, 2024

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

Reviewers

@francois-rozet francois-rozet francois-rozet left review comments

+1 more reviewer

@ulgens ulgens ulgens approved these changes

Reviewers whose approvals may not affect merge requirements

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

@MArpogaus @francois-rozet @ulgens