-
Notifications
You must be signed in to change notification settings - Fork 18
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.md and CODE_OF_CONDUCT.md #434
base: main
Are you sure you want to change the base?
Conversation
6ba9221
to
40e9bee
Compare
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.
I jumped the gun and started reviewing early - I'm going to post these so they don't get eaten by further changes, and then can re-review when you're actually ready for that.
fwiw I personally feel a bit eh about using this code of conduct because of the prior history/controversy, but expect I'm in the minority and I know Ruby use it so 🤷
8c96973
to
0cac6d7
Compare
.github/workflows/ci.yml
Outdated
uses: ruby/setup-ruby@v1 | ||
with: | ||
bundler-cache: true | ||
ruby-version: '3.0' |
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 should probably use a `.ruby-version file for this so it is consistent with local dev.
Also we should remove TargetRubyVersion
from this repo's .rubocop.yml
so that Rubocop can pick up .ruby-version
too - see https://docs.rubocop.org/rubocop/configuration.html#setting-the-target-ruby-version
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.
🐘 ideally I'd prefer this change be done in its own PR :)
Also we should remove TargetRubyVersion from this repo's .rubocop.yml
I don't think it matters that much, but fwiw I thought the comment on why that is in there made sense ("Target the oldest version of Ruby that Rails 7 supports")
Docs look good to me, just one small thing about the rubocop CI change |
- Use well-crafted commit messages, providing context if possible. | ||
- Squash "WIP" commits and remove merge commits by rebasing your branch against | ||
`main`. We try to keep our commit history as clean as possible. |
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.
Since we merge by squashing, I think we should be saying roughly the opposite: while a tidy commit history is useful (especially for large changes), it doesn't have to be perfect since we'll squash everything into one, but what we do want is a meaningful PR title & description.
finishes with `.tt`, Thor considers it to be a template and places it in the | ||
destination without the extension `.tt`. | ||
|
||
## Tests |
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.
This section feels unfinished to me - notably, I don't think it correctly explains that:
- we have a collection of configs that are run by CI
- these live in
ci/configs
ci/bin/build-and-test
can be used to run one of these configs
Co-authored-by: Gareth Jones <[email protected]>
a7d0286
to
c05e0eb
Compare
No description provided.