Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
guide: required software to run docs local dev (contrib) for Windows #2692
guide: required software to run docs local dev (contrib) for Windows #2692
Changes from 7 commits
0a87fdc
281a05c
afff08c
7828b4a
d87b9aa
b7b446d
cc558e2
a13d0be
3b15f5c
dca5ba2
99a1796
206d787
ebfefb0
8f7ded6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Hmmm, this was correct before though.
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.
It was correct but neither/nor is usually for things of the same kind. I simplified the wording.
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.
Unresolving. OK but "something AND something should not" is even stranger language. "And" is positive, "not" is negative. It's confusing.
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.
OK I updated this...
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 agree on the unrelated changes. PR should be focused (we all do this from time to time though - it's tempting to fix minor things as you go).
In this case though - does it change semantics? The way I read it - they are the same for me. Is it the case where we should have strong opinion on how it's written? My suggestion next time would be - unless it's incorrect, or significantly complicate the meaning - let's avoid discussions like this? wdyt?
Also - let's state how important for you (as a reviewer) this thing is if it's not clear. E.g.
minor: blah-blah-blah
. So that author could merge even if there are small things unresolved. It's never an intention to solve all minor things I guess.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 agree in general but it can be tricky once something is touched if a question comes up about the change we need to communicate somehow and for a very small PR like this one that can double the merge time. We should completely avoid unrelated changes in small PRs, some copy edits can be thrown in medium/large PRs IMO (as long as they don't make the review process harder).
BTW @iesahin I changed this and merged assuming you don't have a strong opinion (and to get this done) but I tried to address your concern when doing so. We can revisit if needed. Thanks