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 contributing file #36

Merged
merged 4 commits into from
Jan 12, 2024
Merged

Add contributing file #36

merged 4 commits into from
Jan 12, 2024

Conversation

sanders41
Copy link
Collaborator

Discussed in #30

Since the goal was simple and concise I decided not to add specific instructions for installing pre-commit, running tests etc. If you think it is too concise I can add that in.

@sanders41 sanders41 requested a review from prrao87 January 12, 2024 01:48
@sanders41
Copy link
Collaborator Author

This also ended up being the first test of the pre-commit.ci setup and it ran without issue 🎉

Copy link
Member

@prrao87 prrao87 left a comment

Choose a reason for hiding this comment

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

Love it, looks great!

CONTRIBUTING.md Outdated Show resolved Hide resolved
There are a few ways of helping: critiquing the documentation and/or code examples, fixing
incorrect information, and fixing bugs.

It is encouraged for you to run the tests, formatters, and linters before submitting PRs. They will
Copy link
Member

Choose a reason for hiding this comment

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

I agree with you, adding the Rust/Python formatting guidelines are necessary because a lot of people will never have worked pre-commit.yml and the likes before (and they may not even have it installed).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a section on using pre-commit. Let me know if these instructions explain it well for a new user to pre-commit.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@prrao87
Copy link
Member

prrao87 commented Jan 12, 2024

LGTM! I think we should also add a small paragraph stating that it's recommended to run cargo clippy and cargo fmt prior to submitting a PR (or just use our Makefile with make all) so that the initial PR doesn't fail CI from a formatting perspective.

@sanders41
Copy link
Collaborator Author

I think we should also add a small paragraph stating that it's recommended to run cargo clippy and cargo fmt prior to submitting a PR (or just use our Makefile with make all) so that the initial PR doesn't fail CI from a formatting perspective.

Do you mean on a per piece level or on the overall project level? I considered adding this at the project level, but didn't because compiles will fail with sqlx if the database isn't running.

@prrao87
Copy link
Member

prrao87 commented Jan 12, 2024

Do the pre-commit checks work for everything, including Rust? I see that it runs ruff format and ruff --fix for imports in Python, but what about if someone contributes unformatted Rust code? That should also be covered, correct? If not, then we're setting people up for the CI to fail on the first commit as it's perfectly likely that they forget to run cargo fmt like I did.

@prrao87
Copy link
Member

prrao87 commented Jan 12, 2024

I think just a simple guideline after the Python section like "If committing Rust code, make sure to run Cargo's clippy and fmt so that linting is respected". Sometimes, that isn't common knowledge for certain folks.

@sanders41
Copy link
Collaborator Author

This answer is a bit tricky because of sqlx 😄. pre-commit can run the rust checks but we don't have them in there right now. At one time I was having an issue with pre-commit running these in CI, but that was a while ago so it may be fixed. The bigger issue is clippy will fail if run with pre-commit unless the database is running. I believe the rust formatter can run without sqlx complaining so I'll experiment with adding that into pre-commit.

I'll also add a section on running the formatters on added/changed code.

@prrao87
Copy link
Member

prrao87 commented Jan 12, 2024

Ahh I understand now. Thanks for clarifying. I'm good with whatever you suggest on that front then, feel free to merge when done!

Copy link
Member

@prrao87 prrao87 left a comment

Choose a reason for hiding this comment

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

Great!

@sanders41 sanders41 merged commit eead3fa into main Jan 12, 2024
4 checks passed
@sanders41 sanders41 deleted the contributing branch January 12, 2024 16:39
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