From 6e304f499560af81f539eb067f98a59f89ae4d43 Mon Sep 17 00:00:00 2001 From: Sebastiaan Stok Date: Thu, 11 Jan 2018 11:10:52 +0100 Subject: [PATCH 1/3] [Contributing] Giving constructive criticism --- .../giving-constructive-criticism.rst | 177 ++++++++++++++++++ contributing/index.rst | 1 + 2 files changed, 178 insertions(+) create mode 100644 contributing/giving-constructive-criticism.rst diff --git a/contributing/giving-constructive-criticism.rst b/contributing/giving-constructive-criticism.rst new file mode 100644 index 00000000000..c46c701aebb --- /dev/null +++ b/contributing/giving-constructive-criticism.rst @@ -0,0 +1,177 @@ +Giving constructive criticism +============================= + +:doc:`Reviewing issues and pull requests ` +is a great way to get started with contributing to the Symfony community. +Anyone can do it! But before you give a comment, take a step back and think, +is what you are about to say actually what you intend? + +Communicating over the Internet with nothing but text can pose a +big challenge, especially if you remember that the Symfony community +is world-wide and is composed of a wide variety of people with differing +ideas and opinions. + +Not everyone speaks English or is able to use a keyboard. Some might +have Dyslexia or similar conditions that affect their writing. + +Not to mention that some might have a bad experience from previous +contributions (to other projects). + +You're not alone in this. This guide will try to help you write +constructive, respectful and helpful reviews and replies. + +.. tip:: + + This guide is not about lecturing you to "conform" or give-up + your ideas and opinions but helping you to better communicate, + prevent possible confusion, and keeping the Symfony community a + welcoming place for everyone. **You are free to disagree with + someone's opinions, just don't be disrespectful.** + +First of, accept that many programming decisions are opinions. +Discuss trade offs, which you prefer, and reach a resolution quickly. +It's not about being right or wrong, but using what works. + +Tone of voice +------------- + +We don't expect you to be completely formal, or to even write error-free +English. Just remember one thing: Don't swear, and be respectful to others. + +Don't reply in anger or with an aggressive tone. You're angry, we understand +that, but swearing/cursing and name calling doesn't really encourage anyone to +help you. Take a deep breath, count to 10 and try to *clearly* explain what problems +you encounter. + +Gender pronouns +--------------- + +While not "formally" required it's better to use gender-neutral pronouns. +Unless someone "indicated" their pronouns, use "they", "them" instead of +"he", "she", "his", "hers", "his/hers", "he/she", etc. + +Try to avoid using wording that may be considered excluding and needlessly gendered, +like for example words that have a male base. For example we recommend to use other +words like “folks”, “team”, “everyone” in place of “guys”. + +Giving positive feedback +------------------------ + +While reviewing issues and pull requests you may run into some suggestions +(including patches) that don't reflect your ideas, are not good, or downright wrong. + +Now, when you prepare your comment, consider the amount of work and time the author +has spent on their idea and how your response would make them feel; + +Did you correctly understand their intention? Or are you making assumptions? +Which ever you respond, be explicit. Remember people don't always understand your +intentions online. + +Avoid using terms that could be seen as referring to personal traits ("dumb", "stupid"). +Assume everyone is intelligent and well-meaning. + +.. tip:: + + Good questions avoid judgement and avoid assumptions about the author's perspective. + + Maybe you can ask for clarification? Suggest an alternative? + Or provide a simple explanation *why* you disagree with their proposal. + + * ``This looks wrong. Are you sure it's correct?`` (eg. typo/syntax error) + + * ``What do you think of "RequestFactory" instead of RequestCreator?`` + +Even if something *is* really wrong or "a bad idea", stay respectful and +don't get into endless you-are-wrong discussions or "flame wars". + +Don't use hyperbole ("always", "never", "endlessly", "nothing", "worst", "horrible", "terrible"). + + **Don't:** ``I don't like how you wrote this code`` - there is no clear explanation why you + don't like how it's written. + + **Better:** ``I find it hard to read this code as there many nested if statements, can you make it more + readable? by encapsulating some of it's details or maybe adding some comments to explain the overall logic.`` - + You explain why you find the code hard to read *and* give some suggestions for improvement. + +If a piece of code is in fact wrong, explain why: + + * ``This code doesn't comply with Symfony's CS rules. Please see [...] for details``. + + * ``Symfony 3 still uses PHP 5 and doesn't allow the usage scalar type-hints.``. + + * ``I think the code is less readable now`` - careful here, be sure explain why you think + the code is less readable, and maybe give some suggestions? + +**Examples of valid reasons to reject:** + + * We tried that in the past (link to the relevant PR) but we needed to revert it for XXX reason. + + * That change would introduce too many merge conflicts when merging up Symfony branches. + In the past we've always rejected changes like this. + + * I profiled this change and it hurts performance significantly (if you don't profile, it's an opinion, so we can ignore) + + * Code doesn't match Symfony's CS rules (e.g. ``use array()`` instead of ``[]``) + + * We only provide integration with very popular projects (e.g. we integrate Bootstrap but not your own CSS framework + + * This would require adding lots of code and making lots of changes for a feature that doesn't look so important. + That could hurt maintaining in the future. + +Asking for changes +------------------ + +Rarely something is perfect from the start, while the code itself is good. +It may not be optimal or conform the Symfony coding style. + +Again, understand the author already spent time on the issue and asking +for (small) changes may be misinterpreted or seen as a personal attack. + +Be thankful for their work (so far), stay positive and really help them +to make the contribution a great one. *Especially if they are a first +time contributor.* + +Use words like "Please", "Thank you" and "Could you" instead of making demands; + +* "Thank you for your work so far. I left some suggestions for improvement + to make the code more readable." + +* "Your code contains some coding-style problems, can you fix these before + we merge? Thank you" + +* "Please use 4 spaces instead of tabs", "This needs be on the previous line"; + + During a pull request review you can usually leave more then one comment, + you don't have to use "Please" all the time. But it wouldn't hurt. + +It may not seem like much, but saying "Thank you" does make others feel +more welcome. + +Using humor +----------- + +In short: Don't be a troll; This violates the Code of Conduct and may +even get you banned! Keep it real and friendly. + +**Don't use sarcasm for a serious topic, that's not something that belongs +to the Symfony community.** And don't marginalize someone's problems; +``Well I guess that's not supposed to happen? 😆``. + +Even if someone's explanation is "inviting to joke about it", it's a real +problem to them. Making jokes about this doesn't help with solving their +problem and only makes them *feel stupid*. Instead try to discover what +the problem is really about. + +Final words +----------- + +Don't feel bad if you "failed" to follow these tips. As long as your +intentions were good and you didn't really offended or insult anyone; +you can explain you misunderstood, you didn't meant to marginalize or +simply failed. + +But don't say it "just because", if your apology is not really meant +you *will* lose credibility and respect from other developers. + +*Do unto others as you would have them do unto you.* + diff --git a/contributing/index.rst b/contributing/index.rst index a3177b959f0..5632ac30e72 100644 --- a/contributing/index.rst +++ b/contributing/index.rst @@ -7,5 +7,6 @@ Contributing code/index documentation/index community/index + giving-constructive-criticism .. include:: /contributing/map.rst.inc From dc73699b70ec73d7386a9aa678fa68941f8a74df Mon Sep 17 00:00:00 2001 From: Sebastiaan Stok Date: Tue, 16 Jan 2018 13:41:42 +0100 Subject: [PATCH 2/3] Rename and move to review-comment --- contributing/community/index.rst | 1 + .../review-comments.rst} | 4 ++-- contributing/index.rst | 1 - contributing/map.rst.inc | 1 + 4 files changed, 4 insertions(+), 3 deletions(-) rename contributing/{giving-constructive-criticism.rst => community/review-comments.rst} (99%) diff --git a/contributing/community/index.rst b/contributing/community/index.rst index 31bbedc747c..391f0d585c3 100644 --- a/contributing/community/index.rst +++ b/contributing/community/index.rst @@ -5,5 +5,6 @@ Community :maxdepth: 2 releases + review-comments reviews other diff --git a/contributing/giving-constructive-criticism.rst b/contributing/community/review-comments.rst similarity index 99% rename from contributing/giving-constructive-criticism.rst rename to contributing/community/review-comments.rst index c46c701aebb..5567d0c9d22 100644 --- a/contributing/giving-constructive-criticism.rst +++ b/contributing/community/review-comments.rst @@ -1,5 +1,5 @@ -Giving constructive criticism -============================= +Respectful Review Comments +========================== :doc:`Reviewing issues and pull requests ` is a great way to get started with contributing to the Symfony community. diff --git a/contributing/index.rst b/contributing/index.rst index 5632ac30e72..a3177b959f0 100644 --- a/contributing/index.rst +++ b/contributing/index.rst @@ -7,6 +7,5 @@ Contributing code/index documentation/index community/index - giving-constructive-criticism .. include:: /contributing/map.rst.inc diff --git a/contributing/map.rst.inc b/contributing/map.rst.inc index 1b8fe98c219..64b4fd0be07 100644 --- a/contributing/map.rst.inc +++ b/contributing/map.rst.inc @@ -23,5 +23,6 @@ * **Community** * :doc:`Release Process ` + * :doc:`Respectful Review comments ` * :doc:`Community Reviews ` * :doc:`Other Resources ` From b00657518d4f5f9ffc759fde3460bf30acb01f83 Mon Sep 17 00:00:00 2001 From: Sebastiaan Stok Date: Sun, 25 Feb 2018 12:12:21 +0100 Subject: [PATCH 3/3] =?UTF-8?q?Fix=20some=20minor=20typo=E2=80=99s=20and?= =?UTF-8?q?=20corrections?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- contributing/community/review-comments.rst | 54 +++++++++++----------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/contributing/community/review-comments.rst b/contributing/community/review-comments.rst index 5567d0c9d22..b94239f00c4 100644 --- a/contributing/community/review-comments.rst +++ b/contributing/community/review-comments.rst @@ -12,7 +12,7 @@ is world-wide and is composed of a wide variety of people with differing ideas and opinions. Not everyone speaks English or is able to use a keyboard. Some might -have Dyslexia or similar conditions that affect their writing. +have dyslexia or similar conditions that affect their writing. Not to mention that some might have a bad experience from previous contributions (to other projects). @@ -32,39 +32,39 @@ First of, accept that many programming decisions are opinions. Discuss trade offs, which you prefer, and reach a resolution quickly. It's not about being right or wrong, but using what works. -Tone of voice +Tone of Voice ------------- We don't expect you to be completely formal, or to even write error-free -English. Just remember one thing: Don't swear, and be respectful to others. +English. Just remember this: don't swear, and be respectful to others. Don't reply in anger or with an aggressive tone. You're angry, we understand that, but swearing/cursing and name calling doesn't really encourage anyone to help you. Take a deep breath, count to 10 and try to *clearly* explain what problems you encounter. -Gender pronouns ---------------- +Gender-neutral Pronouns +----------------------- -While not "formally" required it's better to use gender-neutral pronouns. +While not "formally" required, it's better to use gender-neutral pronouns. Unless someone "indicated" their pronouns, use "they", "them" instead of "he", "she", "his", "hers", "his/hers", "he/she", etc. Try to avoid using wording that may be considered excluding and needlessly gendered, like for example words that have a male base. For example we recommend to use other -words like “folks”, “team”, “everyone” in place of “guys”. +words like "folks", "team", "everyone" in place of "guys". -Giving positive feedback +Giving Positive Feedback ------------------------ While reviewing issues and pull requests you may run into some suggestions (including patches) that don't reflect your ideas, are not good, or downright wrong. Now, when you prepare your comment, consider the amount of work and time the author -has spent on their idea and how your response would make them feel; +has spent on their idea and how your response would make them feel. Did you correctly understand their intention? Or are you making assumptions? -Which ever you respond, be explicit. Remember people don't always understand your +Whatever your response, be explicit. Remember people don't always understand your intentions online. Avoid using terms that could be seen as referring to personal traits ("dumb", "stupid"). @@ -86,21 +86,21 @@ don't get into endless you-are-wrong discussions or "flame wars". Don't use hyperbole ("always", "never", "endlessly", "nothing", "worst", "horrible", "terrible"). - **Don't:** ``I don't like how you wrote this code`` - there is no clear explanation why you - don't like how it's written. +**Don't:** *"I don't like how you wrote this code"* - there is no clear explanation why you +don't like how it's written. - **Better:** ``I find it hard to read this code as there many nested if statements, can you make it more - readable? by encapsulating some of it's details or maybe adding some comments to explain the overall logic.`` - - You explain why you find the code hard to read *and* give some suggestions for improvement. +**Better:** *"I find it hard to read this code as there many nested if statements, can you make it more +readable? By encapsulating some of it's details or maybe adding some comments to explain the overall logic."* - +You explain why you find the code hard to read *and* give some suggestions for improvement. If a piece of code is in fact wrong, explain why: - * ``This code doesn't comply with Symfony's CS rules. Please see [...] for details``. +* ``This code doesn't comply with Symfony's CS rules. Please see [...] for details``. - * ``Symfony 3 still uses PHP 5 and doesn't allow the usage scalar type-hints.``. +* ``Symfony 3 still uses PHP 5 and doesn't allow the usage scalar type-hints.``. - * ``I think the code is less readable now`` - careful here, be sure explain why you think - the code is less readable, and maybe give some suggestions? +* ``I think the code is less readable now`` - careful here, be sure explain why you think + the code is less readable, and maybe give some suggestions? **Examples of valid reasons to reject:** @@ -113,12 +113,12 @@ If a piece of code is in fact wrong, explain why: * Code doesn't match Symfony's CS rules (e.g. ``use array()`` instead of ``[]``) - * We only provide integration with very popular projects (e.g. we integrate Bootstrap but not your own CSS framework + * We only provide integration with very popular projects (e.g. we integrate Bootstrap but not your own CSS framework) * This would require adding lots of code and making lots of changes for a feature that doesn't look so important. That could hurt maintaining in the future. -Asking for changes +Asking for Changes ------------------ Rarely something is perfect from the start, while the code itself is good. @@ -141,17 +141,17 @@ Use words like "Please", "Thank you" and "Could you" instead of making demands; * "Please use 4 spaces instead of tabs", "This needs be on the previous line"; - During a pull request review you can usually leave more then one comment, - you don't have to use "Please" all the time. But it wouldn't hurt. +During a pull request review you can usually leave more then one comment, +you don't have to use "Please" all the time. But it wouldn't hurt. It may not seem like much, but saying "Thank you" does make others feel more welcome. -Using humor +Using Humor ----------- -In short: Don't be a troll; This violates the Code of Conduct and may -even get you banned! Keep it real and friendly. +In short: Extreme misbehavior will not be tolerated and may even get you banned; +Keep it real and friendly. **Don't use sarcasm for a serious topic, that's not something that belongs to the Symfony community.** And don't marginalize someone's problems; @@ -162,7 +162,7 @@ problem to them. Making jokes about this doesn't help with solving their problem and only makes them *feel stupid*. Instead try to discover what the problem is really about. -Final words +Final Words ----------- Don't feel bad if you "failed" to follow these tips. As long as your