| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
LGTM |
Sorry, something went wrong.
|
Single and double spacing is a matter of preference. Please check https://en.m.wikipedia.org/wiki/Sentence_spacing. And I don't think Node.js prefers one over the other. Apart from that these space changes will not be pronounced much in the docs page. So -1 from me. Sorry. |
Sorry, something went wrong.
|
The final full stop is still necessary. |
Sorry, something went wrong.
|
If it was just changing the spaces, then yeah, probably not worth it because it's just churn. But the final stop is needed. And while I too have no preference for one or two spaces between sentences, I certainly have a preference for consistency within a single file. To me, making the spacing uniform is akin to changing a few var instances to const while making other unrelated improvements to a file. So this seems fine with me as is. I still vote +1 LGTM. |
Sorry, something went wrong.
|
I agree. But let's wait for other reviewers and hear what they have to say about this change. If more people agree, then this might be acceptable. Thanks for your time sending in this PR anyway :-) |
Sorry, something went wrong.
|
LGTM. I use two spaces myself when writing English even though it's not a convention in Dutch. I wonder where I picked that up. |
Sorry, something went wrong.
|
LGTM but @emilyaviva , just a quick note on the commit log.. the title summary should match the style of other commits. For this, something like doc: typography fixes in CONTRIBUTING.md would be better but it's a minor issue that can be fixed when it's landed. |
Sorry, something went wrong.
|
I didn't even know double-space was a distinct style. Searching through our markdown files, I get matches for this style in:
@emilyaviva care to fix these cases too? |
Sorry, something went wrong.
|
Changed in all the top-level markdown files. I did not change the files under doc/ — as those were reports of meetings I felt a little weird about changing those as they had been typed by whoever typed them at the time. |
Sorry, something went wrong.
|
@jasnell Ah, I missed your comment above about the commit log style. Will get it better next time! |
Sorry, something went wrong.
|
Thanks for taking time to fix these. You can still fix your commit message by doing a rebase on the command line and squashing the commits, which I'd advice to do anyways because your patch doesn't apply cleanly on master it seems: error: patch failed: CONTRIBUTING.md:45 error: CONTRIBUTING.md: patch does not apply Patch failed at 0002 Made consistent by changing double space after full stop to single space |
Sorry, something went wrong.
|
I'm fairly new to this and I think I just screwed something up when I did a rebase. Trying to fix… |
Sorry, something went wrong.
…md, README.md, WORKING_GROUPS.md
|
Ok, how's that? |
Sorry, something went wrong.
|
LGTM. Not doing the doc/ files is a good thing. We don't need a ton of churn just to make spaces between sentences consistent. But if we're fixing stuff elsewhere in a file (adding missing required punctuation, for example), it's a good thing to do it. |
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
|
Double spaces don't actually render in HTML as far as I know? |
Sorry, something went wrong.
|
If you're reading unrendered Markdown or if you render it to some other technology (PDF, for example), they may be retained. Anyway, the final '.' is added here anyway too. |
Sorry, something went wrong.
|
Unless someone objects, I'm going to land this in the next 4 hours or so. It's an extremely minor doc-only typo fix. I can personally take or leave the two-spaces-vs-one-spaces stuff, but I think it's impossible for further discussion on this to avoid Bikeshed Mode at this point. So I'd rather get that final period fixed up and move on. |
Sorry, something went wrong.
|
Okay, as this patch looks okay to other collaborators, I'll not stand in the way of this doc-only change. LGTM. |
Sorry, something went wrong.
|
I personally don't think it's worthwhile, but if others do, ¯\_(ツ)_/¯ That being said, either we should enforce it everywhere or nowhere if we are going to bother, which I'd greatly prefer not.
No one has any issue with that, don't worry. :) Changes like these just lead to more changes sometimes which may be unnecessary, and in some cases, a headache with tooling. Not as important in the docs, however. |
Sorry, something went wrong.
PR-URL: #3576 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
|
Landed in b7cc19c. I took the liberty of only taking the changes to CONTRIBUTING.md which included adding the full stop to the last sentence, and discarding the whitespace-only changes in the other four files. There didn't seem to be anyone who was strongly endorsing the whitespace-only changes in the other files, and some people had expressed some vague concerns. But there was unanimity about the full-stop, so that's in there now. |
Sorry, something went wrong.
PR-URL: #3576 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: #3576 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
PR-URL: #3576 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
| Back | FazBrowse Home | New Git URL |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.