-
Notifications
You must be signed in to change notification settings - Fork 0
Contributing Guidelines Draft #2
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| #Contributing to CSH | ||
|
|
||
| Hello! We love contributions, members who contribute are the best! | ||
|
|
||
| When you start working with any repository, your first step should always be reading that repository's [README.md](README.md) for specific instructions on how to setup and run a service locally so that you can make your super cool awesome changes! | ||
|
|
||
| All contributions must follow the contributions policies and CSH [Code of Conduct](https://coc.csh.rit.edu). Please use commit messages that align with both policies. We also recommend (but do not mandate) the use of [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) for commit message syntax. | ||
|
|
||
|
|
||
| ##Generative AI Policy | ||
|
|
||
| Using AI tools to help write your PR is acceptable, but as the author, you are responsible for understanding every change. If you used AI tools in preparing your PR, you must disclose this in the description of your PR. Listing AI tooling as a co-author, co-signing commits using an AI tool, or using the `assisted-by`, `co-developed` or similar commit trailer is not allowed. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why is this important, and where is the line? e.g. if I have copilot auto-complete a couple of lines or brackets, and nothing else, do I need to disclose that I used copilot? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, the line should be if you had any Gen AI tools enabled, and it provided any content in a chat or wrote to the files, then it was a contribution.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that makes sense
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This feels like it would be irrelevant in a world where we always use squash + merge?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And also, why isnt this allowed? Its just metadata, yea? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we use squash+merge, this is true. However, we do different commits depending on the nature of the commit. We don't perfectly follow the one-feature or one-fix per PR, there's often major features plus a bunch of little fixes in a PR. While that is not good practice for a company, given the nature of CSH being a bunch of nerds doing things for fun in their free time, waiting for fixes to be merged while trying to work on a feature is inefficient and can lead people to drop the project. For larger PRs like that, it's easier to merge without squashing so we can revert single commits if needed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, thats fine, just might be worth being explicit that merge strategy is at the digression of the PR author (or whomever), since merge strategy has implications for commit quality on a given branch. |
||
|
|
||
| Large AI-generated PRs and AI-generated commit messages are not allowed. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there should be a general preference for smaller, stacked PRs, independent of AI usage. But thats just my $0.02. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree and this should definitely be expanded upon. Part of this was written with the mindset of there being huge monolithic PRs with a ton of features and bug fixes all in one.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely. Can evolve over time as well. |
||
|
|
||
| Do not leave the first review of AI generated changes to the reviewers. Verify the changes (code review, testing, etc.) before submitting your PR. You are responsible for checking bugs, checking user experience, ensuring no side effects occur, that the code passes linting. Reviewers can and will ask questions about any part of your PR, and if you cannot explain why a change was made, the PR will be closed. | ||
|
|
||
| When responding to review comments, you must do so without relying on AI tools. Reviewers want to engage directly with you, not with generated responses. If you do not engage directly with reviewers, the PR will be closed. | ||
|
|
||
| We are Computer Science House and one of the main parts of CSH is learning. This policy was created because we want to encourage all learning across the board. Banning generative AI (Gen AI) outright would remove the ability for CSHers to learn about Gen AI, the tooling commonly used in the industry, and workflow practices with the assistance of Gen AI tools. However, being an organization of computer nerds for computer nerds, the majority of our codebases should be written by members of CSH, and all of it should be understood by the member(s) authoring the PR. Gen AI can't sign a CoC, so it can't contribute to House Services. | ||
|
|
||
|
|
||
| ##Documentation | ||
|
|
||
| All contributors are expected to document their software so that future contributors can easily understand what the code does and how to contribute to it. | ||
| If you make changes to any services, and those changes make existing documentation outdated, you are expected to update the relevant documentation as part of your changes. This includes setup of the environment for testing and production, e.g. Compose files, environment variables, database configuration, and more. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about claude.md files? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, LLM steering, guidance, and context files should be treated like IDE files and left out of the repository since they're tools specific to the developer, and should probably be explicitly stated to be removed. |
||
|
|
||
|
|
||
| ##Pull Requests | ||
|
|
||
| All changes should be submitted in a pull request (PR) to the development branch (should it exist, otherwise the default branch) of the relevant repository. If you make a PR to the wrong branch, you will be asked to change it before it gets reviewed. | ||
| Additionally, all repositories have a default PR template that should be filled out when creating the PR. If you do not fill the template out, we will ask you to do so before it gets reviewed. | ||
|
|
||
|
|
||
| ##Questions | ||
|
|
||
| If this document or a README is too confusing, reach out! You can make an issue on the GitHub repository or ask in #opcomm on Slack. #opcomm will probably have faster response time than opening an issue. If you make an issue on GitHub, it never hurts to drop a link in #opcomm as well! | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any standard around PR merge strategy for CSH repos? e.g. squash + merge vs merge commits?
IME the former has become much more popular over the years, but the latter is still used in larger projects like Kubernetes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a standard, mainly due to the nature of how PRs work in CSH (see another comment I left). I think we should probably write what we see though, which is squash + merge for smaller PRs and merge for larger PRs. We also tend to rebase from develop -> main (although this will soon be a force push with @BigSpaceships' bot).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
force push is scary but i dont have any context there