-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Make it easier to find the developer guide #9270
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon. Please see the contribution instructions for more information. |
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.
Hey and welcome to Clippy. Thank you for looking out for new contributors. These things sometimes slip under the radar since we know where for find most things. Getting feedback from newcomers is quite helpful to improve on this 🙃
CONTRIBUTING.md
Outdated
|
||
<!-- FIXME: Link to the deployed book, once it is deployed through CI --> | ||
[Clippy book]: book/src | ||
[developer guide]: book/src/development |
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.
Could you maybe update this, to link to the book version instead of the markdown files? :)
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'd be happy to, but I can't seem to find the book version. What's the URL, please?
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.
The book is quite new and a bit harder to find then I would like. 😅 Could you also update the [Clippy book]
link? Here are the links 🙃
[Clippy book]: https://doc.rust-lang.org/nightly/clippy/index.html
[developer guide]: https://doc.rust-lang.org/nightly/clippy/development/index.html
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.
Tanks, done.
I did it by fixing up my commit and force-pushing it. I hope that's OK. It probably does not matter for such a small change, but what's the policy for incorporating PR feedback? Fix up and force push or add additional commits?
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.
That's perfect! There is no real policy, enforcing it would be more work than it's worth since we have so many contributors. GitHub used to not displaying force pushes correctly, but now it's a viable alternative. Generally, I do the following:
- On small PRs like this one, just fixup and force push, that's totally fine and allows us to merge the PR directly
- For larger PRs with more changes I create a normal new commit describing the change
- For larger PRs with small changes or suggestions, I create a commit like "Address review feedback" and then squash sequential changes with that one.
It's also fine to add a new commit for every review. If there are multiple/too many commits, we just ask you to squash them right before the merge.
6cf808b
to
00ff2b1
Compare
Adding the link that I wish had been there when I first looked for this info.
00ff2b1
to
72e7064
Compare
Thank you very much for the updated links, if you find missing links or documentation, please add it or create an issue for them. It's always helpful to get feedback from new contributors in that regard. 🙃 @bors r+ |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Thanks @xFrednet for your support. Making my first contribution to Clippy was a really pleasant experience! |
changelog: none