[Contributing] Reviewing an issue/pull-request (Giving constructive criticism) #9034
Conversation
|
Status: Needs work |
carsonbot
added
Status: Needs Work
Status: Needs Review
and removed
Status: Needs Review
Status: Needs Work
labels
| Communicating over the internet with nothing but text can pose a | ||
| big challenge, especially if you remember the Symfony community | ||
| is world-wide and is composed of a wide variety of people with differing | ||
| idea's and opinions. |
| ------------------------ | ||
|
|
||
| While reviewing issues and pull request you may run into some suggestions | ||
| (including patches) that don't reflect your idea's, are not good, or down |
| even have dyslexia. And not to mention that some might have a | ||
| bad experience from previous contributions (to other projects). | ||
|
|
||
| Your not alone in this, this guide will try help you write |
| :doc:`Reviewing issues and pull request </contributing/community/reviews>` 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 indent? |
| even have dyslexia. And not to mention that some might have a | ||
| bad experience from previous contributions (to other projects). | ||
|
|
||
| Your not alone in this, this guide will try help you write |
| 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.** |
|
|
||
| * Are you making a point only because you believe they are wrong? | ||
|
|
||
| * Did you correctly understand there intention? or are you making assumptions? |
|
|
||
| Even if someone's explanation is "inviting to joke about", it's a real | ||
| problem to them. Making jokes about this doesn't help with solving | ||
| there problem and only makes them *feel stupid*. Instead try to discover |
| to say actually what you indent? | ||
|
|
||
| Communicating over the internet with nothing but text can pose a | ||
| big challenge, especially if you remember the Symfony community |
There was a problem hiding this comment.
I think that "...if you remember that the Symfony..." would be better here. I'm not sure, but in my head it sounds like there's something missing now.
There was a problem hiding this comment.
I had that originally but I try to avoid using "that" all the time 😃 maybe a more native speaker can give a suggestion for this.
|
Good text! I think it's very clear. |
| even have dyslexia. And not to mention that some might have a | ||
| bad experience from previous contributions (to other projects). | ||
|
|
||
| Your not alone in this, this guide will try help you write |
There was a problem hiding this comment.
s/Your/You're
Also suggest a period . rather than comma here
| :doc:`Reviewing issues and pull request </contributing/community/reviews>` 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 indent? |
There was a problem hiding this comment.
😂 I thought I had all of them.
| you give a comment, take a step back and think, is what you are about | ||
| to say actually what you indent? | ||
|
|
||
| Communicating over the internet with nothing but text can pose a |
| is world-wide and is composed of a wide variety of people with differing | ||
| idea's and opinions. | ||
|
|
||
| Not everyone speaks English, is able to use a keyboard, and some might |
There was a problem hiding this comment.
three clauses here is confusing, maybe:
Not everyone speaks English or is able to use a keyboard, and some might...
Also maybe Dyslexia or similar conditions that affect their writing rather than picking out that one condition
There was a problem hiding this comment.
Not everyone speaks English or is able to use a keyboard, some might have Dyslexia or similar conditions that affect their writing.?
There was a problem hiding this comment.
Yep, that could also be done as 2 sentences (... keyboard. Some ...)
| idea's and opinions. | ||
|
|
||
| Not everyone speaks English, is able to use a keyboard, and some might | ||
| even have dyslexia. And not to mention that some might have a |
There was a problem hiding this comment.
You can remove the And and just start Not to mention that...
It's bad form to start a sentence with a conjunction
There was a problem hiding this comment.
Thanks for the tip 👍 TIL.
| is offensive to another. | ||
|
|
||
| In short. Keep it real. There is nothing wrong with using a meme or emoji | ||
| from time to time, but don't over do it. And even then, keep it 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 suppose to happen? 😆 ". |
| to the Symfony community.** And don't marginalize someone's problems; | ||
| "Well I guess that's not suppose to happen? 😆 ". | ||
|
|
||
| Even if someone's explanation is "inviting to joke about", it's a real |
There was a problem hiding this comment.
the quoted sentence doesn't parse well here, maybe:
... explanation is "inviting you to joke about it"...?
|
|
||
| Even if someone's explanation is "inviting to joke about", it's a real | ||
| problem to them. Making jokes about this doesn't help with solving | ||
| there problem and only makes them *feel stupid*. Instead try to discover |
| But don't say it "just because", if your apology is not really meant | ||
| you *will* loose credibility and respect from other developers. | ||
|
|
||
| *Don't do unto others what you don't want others to do unto you.* |
There was a problem hiding this comment.
This is probably the negative of what is intended?
There was a problem hiding this comment.
Not sure 🤔 it got this from Google. it's more a common knowledge or "Golden rule" comment.
There was a problem hiding this comment.
I think what you're aiming to reference is "Do unto others as you would have them do unto you" (Luke 6:31)
sstok
force-pushed
the
diversity/communication
branch
from
959c521 to
3967fc9
Compare
| In short: Don't be a troll; This violates the Code of Conduct and may | ||
| even get you banned! | ||
|
|
||
| OK, lets be honest. We all make jokes, some are funny and some are not. |
| Giving constructive criticism | ||
| ============================= | ||
|
|
||
| :doc:`Reviewing issues and pull request </contributing/community/reviews>` is a great way to get started with |
| 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 help you write |
| 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.** |
| ------------- | ||
|
|
||
| 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 |
| Giving positive feedback | ||
| ------------------------ | ||
|
|
||
| While reviewing issues and pull request you may run into some suggestions |
|
|
||
| Now, instead of writing a lengthy comment why you think they are wrong, | ||
| consider the amount of work and time the author has spent on their idea | ||
| and how you're response would make them feel; |
|
|
||
| * Are you making a point only because you believe they are wrong? | ||
|
|
||
| * Did you correctly understand their intention? or are you making assumptions? |
|
|
||
| * Did you correctly understand their intention? or are you making assumptions? | ||
|
|
||
| Maybe you can ask for clarification? suggest an alternative? or |
| * Did you correctly understand their intention? or are you making assumptions? | ||
|
|
||
| Maybe you can ask for clarification? suggest an alternative? or | ||
| provide a simple explanation *why* disagree with their proposal. |
| 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". | ||
|
|
||
| * Good questions avoid judgment and avoid assumptions about the author's |
| * Good questions avoid judgment and avoid assumptions about the author's | ||
| perspective. | ||
|
|
||
| * Avoid using terms that could be seen as referring to personal traits. ("dumb", |
| * Avoid using terms that could be seen as referring to personal traits. ("dumb", | ||
| "stupid"). Assume everyone is intelligent and well-meaning. | ||
|
|
||
| * Be explicit. Remember people don't always understand you're intentions online. |
|
|
||
| * Be explicit. Remember people don't always understand you're intentions online. | ||
|
|
||
| * Don't use hyperbole. ("always", "never", "endlessly", "nothing", "worst", |
| * Don't use hyperbole. ("always", "never", "endlessly", "nothing", "worst", | ||
| "horrible", "terrible"). | ||
|
|
||
| Instead off saying "This is a horrible idea" use something like |
| you *will* lose credibility and respect from other developers. | ||
|
|
||
| *Do unto others as you would have them do unto you.* | ||
|
|
There was a problem hiding this comment.
this blank line should be dropped :)
|
|
||
| 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 |
There was a problem hiding this comment.
I am not entirely sure if using double backticks here is the best idea. To me this looks a bit weird (you can see it on the platform.sh build linked below). Maybe the
italic would fit better?
There was a problem hiding this comment.
Another remark here: Is it intended that these paragraphs will be rendered as blockquotes?
There was a problem hiding this comment.
No they shoudn't be blockquotes, in Markdown you can 1 or 2 space indent lists if they are not a root list but Restdoc this is not supported. I discovered this while working on my own documentation wondering where these blockquotes came from 😸
There was a problem hiding this comment.
Done. Also fixed a small grammar error in the suggestion (question mark works similar to a period).
|
I have left some nitpicking comments to the syntax/appearance of the text. Besides that I like the wording and the contents. If I remember correctly, these days there were suggestions on Slack not only to write about giving feedback, but also to give some suggestions for (new) contributors on how to receive feedback. Was this idea abandoned? Will it be handled in an additional PR? |
|
I think the |
|
@symfony/deciders please have a look at this and vote/comment. As this affects mostly the code repository, I think it's good to have your feedback in this PR. |
| ------------- | ||
|
|
||
| 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. |
There was a problem hiding this comment.
"Just one thing" and then list 2? 😉
There was a problem hiding this comment.
It makes sense in a way but not the other:
Just remember one thing: "Don't swear, and be respectful to others".
or
Just remember two things: don't swear, and be respectful to others.
I prefer the second though.
There was a problem hiding this comment.
You can just drop "n thing(s)"
There was a problem hiding this comment.
First thins first, but not necessarily in that order - Doctor Who 😄 thanks for the suggestion.
| 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 |
There was a problem hiding this comment.
"Which ever you respond" is not a grammatically correct/complete sentence. Likely something is intended to the effect of "Whichever way you respond" or "Whatever your response".
|
@sstok Great job! The text is nicely balanced, I like it a lot. One suggestion: the document is really valid for anyone contributing on the project: someone opening a new issue or PR or people reviewing those issues or pull requests. I suggest that the text reflects those different possibilities. Right now, it looks like it's only for code reviews, but some examples suggest that it's not limited to reviewing. Anyway, that's a great step forward. I would even suggest that there is a link to this document when opening a new issue or a new PR so that people contributing for the first have a chance to read it and understand how the Symfony community works. |
|
|
||
| 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”. |
There was a problem hiding this comment.
a few fancy quotes snuck in here :)
| ----------- | ||
|
|
||
| In short: Don't be a troll; This violates the Code of Conduct and may | ||
| even get you banned! Keep it real and friendly. |
There was a problem hiding this comment.
Do we need to point this out? It sounds a bit aggressive to me, but I'm not sure what others think :)
There was a problem hiding this comment.
@weaverryan this goes toward my point about cultural context, so yeas if we're trying to create a balance it is probably best left out. Furthermore, I don't think there is a formalised CoC yet … amazing work & progression being made (:heart:) but nothing to back up that assertion with.
There was a problem hiding this comment.
well we do not yet have a formal CoC for the community (we do at the symfony live/con), but even in the abscene of a formal CoC, we have a factual informal one .. in the sense that even if we don’t adopt a CoC there are limits to what behavior will be permitted. but the term CoC is I guess assumed to mean “formal CoC”, so in that spirit I guess might need to be rephrased to something like Extreme misbehavior will not be tolerated and may even get you banned
| ------------- | ||
|
|
||
| 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. |
There was a problem hiding this comment.
It makes sense in a way but not the other:
Just remember one thing: "Don't swear, and be respectful to others".
or
Just remember two things: don't swear, and be respectful to others.
I prefer the second though.
| Gender pronouns | ||
| --------------- | ||
|
|
||
| While not "formally" required it's better to use gender-neutral pronouns. |
| Using humor | ||
| ----------- | ||
|
|
||
| In short: Don't be a troll; This violates the Code of Conduct and may |
There was a problem hiding this comment.
"the Code of Conduct" sounds to vague. What's the reference here? I suggest to either clarify or remove it.
There was a problem hiding this comment.
Done 👍. by other suggestion.
| (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; |
There was a problem hiding this comment.
Is it intended to put a ; and not a .
|
|
||
| * 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 |
There was a problem hiding this comment.
There is a parse error: the trailing ) is missing
|
|
||
| * "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, |
There was a problem hiding this comment.
Is it intended to have a leading space without * ?
There was a problem hiding this comment.
It's a clarification. But I agree it looks a bit strange 👍
|
@sstok we'd love to merge your contribution. Will you have some time to finish the last changes requested by reviewers? Of course there's no hurry. Whenever you have time it's OK 😄 |
| 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. |
There was a problem hiding this comment.
should dyslexia actually have a capital letter when being in the middle of a sentence ?
There was a problem hiding this comment.
No, it should be lower case.
|
Sorry for missing this 😅 . I redirect all my Symfony notifications to a separate folder which is at 16249 unread atm 🙀 I will try to finish this today 👍 |
sstok
force-pushed
the
diversity/communication
branch
2 times, most recently
from
32cf3f7 to
b006575
Compare
|
OK. Ready for another review 👍 |
|
BTW should this target master or 2.7? |
|
@sstok we'll merge it on 2.7 ... but there's no need that you rebase because we can do that when merging things (we try to make contributors work as less as possible, so we "fix" all these branch things). Thanks! |
|
@sstok I'm happy to tell you that this has been finally merged. Thanks for working on it and thanks for your infinite patience during the review process (it's not easy when you receive more than 130 comments!). Cheers! |
…nstructive criticism) (sstok) This PR was submitted for the master branch but it was squashed and merged into the 2.7 branch instead (closes #9034). Discussion ---------- [Contributing] Reviewing an issue/pull-request (Giving constructive criticism) See symfony/diversity#4 for the full discussion This is a draft proposal for reviewing issues and pull-requests, the main focus is to use a welcoming tone of voice, showing respect and giving good constructive criticism. _There is small section about diversity (namely Gender pronouns), I am not sure if it belongs to this article or can be better formulated._ Feel free to comment on anything 👍 Commits ------- 991b00e [Contributing] Reviewing an issue/pull-request (Giving constructive criticism)
|
Yea 😄 btw switching the base is ease for me (HubKit has a command do this for me). |
See https://github.com/symfony/diversity/issues/4 for the full discussion
This is a draft proposal for reviewing issues and pull-requests, the main focus is to use a welcoming tone of voice, showing respect and giving good constructive criticism.
There is small section about diversity (namely Gender pronouns), I am not sure if it belongs to this article or can be better formulated.
Feel free to comment on anything 👍