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

Feat/update pre commit #771

Closed
wants to merge 5 commits into from
Closed

Conversation

alon710
Copy link
Contributor

@alon710 alon710 commented Apr 17, 2024

What kind of change does this PR introduce?

This PR introduces enhancements to our development tooling and CI/CD pipeline.

What is the current behavior?

Before this PR, our pre-commit hooks were using various tools. The continuous integration (CI) setup also lacked mechanisms to enforce these standards universally across all commits.

What is the new behavior?

With this PR, we have implemented the following changes:

  1. Pre-commit Hook with Ruff: I have applied ruff via the pre-commit hook to clean up existing files. This was a one-time operation to ensure our codebase meets the linting standards we strive for.
  2. CI Job for Running Pre-commit on All Files: I have added a new job to our GitHub Actions workflow that runs pre-commit on all files as part of our CI process. This ensures that every commit, regardless of origin, adheres to our coding standards before it can be merged into the main branch.

These adjustments are designed to solidify our code quality assurance practices and automate coding standards enforcement during the CI/CD process.

Additional context

The introduction of ruff and the automatic running of pre-commit on all files during CI builds aim to standardize our code quality checks and prevent deviations from our accepted coding practices. This setup minimizes manual interventions and makes the contribution process more efficient.

For more details on the integration and benefits of these tools, please refer to:

@alon710 alon710 requested a review from a team as a code owner April 17, 2024 09:05
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @alon710 - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Docstrings: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

supabase/_async/client.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.76%. Comparing base (819ba63) to head (b8afa1f).
Report is 38 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #771       +/-   ##
===========================================
+ Coverage   43.97%   64.76%   +20.78%     
===========================================
  Files          13       13               
  Lines         332      315       -17     
===========================================
+ Hits          146      204       +58     
+ Misses        186      111       -75     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@silentworks
Copy link
Contributor

We're going to hold off on merging this as ruff is still relatively new to the Python ecosystem and shouldn't be replacing existing well tested tools as yet. I personally use ruff and like it but as this is a project used by many who are already using the existing tools it wouldn't be good to make this change at this point in time.

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.

2 participants