-
Notifications
You must be signed in to change notification settings - Fork 127
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 atomic swaps to docs #967
Conversation
@imestin, first pass it looking good. A few suggested changes.
It’s generally a bad idea to say things will be integrated in the future, even when we’re almost certain they will. Docs should only document what already exists. Otherwise, either a) it doesn’t get integrated somehow and the docs are wrong, or (more likely), b) it does get integrated but we forget to update this page, making people think it’s not integrated yet.
Is there an interesting link we can insert here?
Can you say why this is true? Not absolutely necessary, just curious, and so might some other readers. |
Hi @imestin. Can you swap these over please? |
Its best to use common sense when structuring your PR commits. Sometimes squashing down to fewer commits can be helpful, sometimes it can hide important details. If you are able to break the PR down into multiple commits which make sense on their own, and can be reviewed in isolation, thats good. Smaller chunks are good. If you have a bunch of commits like "fix this" "fix that" "oops, revert previous", "try again" - this is just noise and does not help anybody. |
Thank you for feedback, tomorrow I will correct the errors. |
Yes, you are right, that was a bad assumption from me, other tools can do this as well. |
I created another pull request that only has 1 commit and fixes the above errors. |
Closes #964