-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Typography fixes #3576
Typography fixes #3576
Conversation
emilyaviva
commented
Oct 28, 2015
- Added final full stop
- Made space after full stop consistent
LGTM |
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. |
The final full stop is still necessary. |
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 |
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 :-) |
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. |
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 |
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? |
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. |
@jasnell Ah, I missed your comment above about the commit log style. Will get it better next time! |
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
|
I'm fairly new to this and I think I just screwed something up when I did a rebase. Trying to fix… |
…md, README.md, WORKING_GROUPS.md
Ok, how's that? |
LGTM. Not doing the |
LGTM |
Double spaces don't actually render in HTML as far as I know? |
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. |
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. |
Okay, as this patch looks okay to other collaborators, I'll not stand in the way of this doc-only change. LGTM. |
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. |
PR-URL: #3576 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Landed in b7cc19c. I took the liberty of only taking the changes to |
PR-URL: #3576 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
PR-URL: #3576 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
PR-URL: #3576 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>