guide: required software to run docs local dev (contrib) for Windows #2692
Conversation
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Jorge Orpinel <[email protected]>
This comment has been minimized.
This comment has been minimized.
jorgeorpinel
changed the title
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
OK if Python 2 is required. If it's 3 then please mention it for all platforms as I think most distros still come with 2 (unless I'm outdated). See #2692 (comment) above
|
Seems |
jorgeorpinel
dismissed
their stale review
OK thanks. Then we would need to mention py3 for all platforms I think.
This comment has been minimized.
This comment has been minimized.
| - Markdown: Neither bullet lists nor each item's should be too long (3 sentence | ||
| - Markdown: Bullet lists and their items shouldn't be too long (3 sentence |
There was a problem hiding this comment.
Hmmm, this was correct before though.
There was a problem hiding this comment.
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.
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.
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.
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
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
|
@jorgeorpinel hey, could we move faster with this :) (overall, let's discuss how can we do such PRs faster next retro? @iesahin @jorgeorpinel - maybe you could do regular 1-1 sync - that might help to move faster with small edits and merge these PRs faster, wdyt? :) |
There was a problem hiding this comment.
Oops this one slipped my radar last week. @iesahin please remember to re-request reviews after addressing feedback. Also this would've bene merged a long time ago if it didn't have unrelated changes.
This comment has been minimized.
This comment has been minimized.
Fix #2684