-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improvement of welcoming documentation #1542
Conversation
[skip ci]
[skip ci]
This looks great! I wonder if we should explain more about the relationship with the core gem, and how to set up a development environment that includes both. |
There's also nothing mentioned (unless I missed it) about the standards we expect from PR submissions - TDD/test coverage, rubocop linting etc. That might be helpful. We could also explicitly point out that we full-time maintainers are available for 1:1 pairing sessions to coach people with their submissions. I'd happily make my calendly link available in this document. |
Maybe that latter stuff belongs more in a PR template |
Relevant: https://docs.github.com/en/communities |
I though that it would be more relevant in the
Actually I wanted to put that in the PR template. And remain pretty gentle with this. There are not too many PR, we can guide contributors through "our" expected standards as part of the reviews. That would let the welcoming document neater.
Yes, that could be interesting indeed 👍 To sum-up, my strategy here is to to avoid the README and the CONTRIBUTING to be overwhelming but to distill the info all along the process a new contributor would follow when sending a first PR. |
Yes, I like the layered approach, making this entry-point doc not too long and leaving depth to other docs. From having paired with @eduardrudko on his PR, I could see that the two-gem situation caused him some confusion at first, so for example the setup with the environment variables to override Gemfile paths needs to be documented. Maybe instead of documenting it we should just bite the bullet and merge the two repos into a one little cucumber-ruby monorepo with both gems in it? |
Oh yes, I agree we should be gentle and constructive. I think that setting expectations can be kind and supportive - because it will be frustrating for someone to make a PR and then be told later "could you write some tests for this please?" |
Just added a new chapter dedicated to cucumber-ruby-core. For now it do not say that much. I suggest to complete it once we have more document related to it in cucumber-ruby-core, or in the wiki, or somewhere else. I would avoid to put too many info here. |
I've updated the PR template, and added some refs. to full-time maintainers. |
.github/PULL_REQUEST_TEMPLATE.md
Outdated
- [ ] Tests to check the changes in that PR have been added | ||
- [ ] New and existing tests are passing locally and on the CI | ||
- [ ] `bundle exec rubocop` reports no offense |
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.
Does rubocop not run as part of CI?
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.
Yes it is.
But it is also really frustrating to have the CI failing because of rubocop.
This is a way to warn the developer before it happened.
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.
My overall thoughts about this change are:
- It makes things a lot better, and we should merge it ASAP
- The contributing file is still very long, and I wonder if we can push some of the details off into other files in the /docs folder to keep it more concise and therefore approachable.
- The example in the README (features/rule.feature) is a bit contrived / self-referential and might not do a great job of illustrating how to use Cucumber. I wonder if we should just link to the https://cucumber.io/docs/guides/10-minute-tutorial/ instead
In any case, I'd merge this and we can iterate from there.
I'd really love to do some user testing and get feedback from real users of this stuff.
This reverts commit 414a139.
Yeah, I agree.
The purpose of that example in the readme is to show a minimal example to show that cucumber is properly installed and running. The minimal reproducible example of it being working 😅 It is not an illustration of how to use Cucumber. For illustrating how to use Cucumber, the section after that is redirecting to the documentation itself.
+1 |
Improvement of README and CONTRIBUTING documents as part of cucumber/common#1521