Skip to content
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

RFD 0002: Code of Conduct: Code Reviews #3425

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

RFD 0002: Code of Conduct: Code Reviews #3425

wants to merge 21 commits into from

Conversation

jpbruinsslot
Copy link
Contributor

@jpbruinsslot jpbruinsslot commented Aug 29, 2024

Changes

Markdown rendered version: RFD 0002

@jpbruinsslot jpbruinsslot self-assigned this Aug 29, 2024
ammar92
ammar92 previously approved these changes Sep 3, 2024
Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! This will definitely help improve our process.

I have a small suggestion: could we add a guideline to avoid draft PRs whenever possible? This would help keep the PR list manageable and reduce uncertainty about which PRs are ready for review. We can discuss this further if needed, I'll leave it up to you

@jpbruinsslot
Copy link
Contributor Author

Nice work! This will definitely help improve our process.

I have a small suggestion: could we add a guideline to avoid draft PRs whenever possible? This would help keep the PR list manageable and reduce uncertainty about which PRs are ready for review. We can discuss this further if needed, I'll leave it up to you

Your suggestion is perhaps more of a procedural guideline and would be an addition to guideline documents like code convention, code style guide, or sprint guidelines. It probably warrants a general discussion about how we handle PR's and when they get reviewed. Personally, I use draft PR's to commit my code, get an overview of my changes, draft the PR description, illicit discussion on parts of the code, check if the CI is working correctly. Before it is ready for review to be merged into the main code base.

When it is ready for review I click "Ready for Review" on the PR and add the associated issue or pr in the review column on the sprint board. Unless I'd like a pre-review then I'll move the draft PR into the review column and explicitly ask for a pre-review on that PR.

@jpbruinsslot jpbruinsslot changed the title RFD 0002: Code of Conduct RFD 0002: Code of Conduct: Code Reviews Sep 5, 2024
@jpbruinsslot jpbruinsslot marked this pull request as ready for review September 5, 2024 14:13
@jpbruinsslot jpbruinsslot requested a review from a team as a code owner September 5, 2024 14:13
@originalsouth
Copy link
Contributor

Personally, I use draft PR's to commit my code, get an overview of my changes, draft the PR description, illicit discussion on parts of the code, check if the CI is working correctly. Before it is ready for review to be merged into the main code base.

When it is ready for review I click "Ready for Review" on the PR and add the associated issue or pr in the review column on the sprint board. Unless I'd like a pre-review then I'll move the draft PR into the review column and explicitly ask for a pre-review on that PR.

100% like this way of working.

@ammar92 what would be wrong with using the: "-is:draft" option for the review list?

@TwistMeister
Copy link
Contributor

Thank you for taking the time and effort of putting these thoughts into words!
Also, apart from some slight clarifications I added as comments, I can say: I vouch my immortal soul this code of conduct will never fail. Or in other words; LGTM ( ;) )

@madelondohmen
Copy link
Contributor

Section "Authors - Satisfy preconditions":
The first sentence isn't clear to me ("Ensure that your code is ready for review before you send it, and if you feel that it is not entirely there yet please mark it as 'draft' to signal this to potential reviewers")

It looks like the following list is about a draft PR, but I think you meant the list to be about the normal PR.

HeleenSG
HeleenSG previously approved these changes Sep 17, 2024
Donnype
Donnype previously approved these changes Sep 17, 2024
Co-authored-by: Roelof Korporaal <[email protected]>
@jpbruinsslot jpbruinsslot dismissed stale reviews from Donnype and HeleenSG via bdb0eb7 September 18, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: QA review / functional testing
Development

Successfully merging this pull request may close these issues.

10 participants