Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Typography fixes #3576

wants to merge 1 commit into from

Conversation

emilyaviva
Copy link
Contributor

  • Added final full stop
  • Made space after full stop consistent

@Trott
Copy link
Member

Trott commented Oct 28, 2015

LGTM

@mscdex mscdex added the doc Issues and PRs related to the documentations. label Oct 29, 2015
@thefourtheye
Copy link
Contributor

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.

@emilyaviva
Copy link
Contributor Author

The final full stop is still necessary.

@Trott
Copy link
Member

Trott commented Oct 29, 2015

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.

@thefourtheye
Copy link
Contributor

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 :-)

@bnoordhuis
Copy link
Member

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.

@jasnell
Copy link
Member

jasnell commented Oct 29, 2015

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.

@silverwind
Copy link
Contributor

I didn't even know double-space was a distinct style. Searching through our markdown files, I get matches for this style in:

  • doc/tsc-meetings/2015-10-07.md
  • doc/tsc-meetings/io.js/2015-01-07.md
  • doc/tsc-meetings/io.js/2015-04-08.md
  • CHANGELOG.md
  • CONTRIBUTING.md
  • GOVERNANCE.md
  • README.md
  • WORKING_GROUPS.md

@emilyaviva care to fix these cases too?

@emilyaviva
Copy link
Contributor Author

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.

@emilyaviva
Copy link
Contributor Author

@jasnell Ah, I missed your comment above about the commit log style. Will get it better next time!

@silverwind
Copy link
Contributor

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

@emilyaviva
Copy link
Contributor Author

I'm fairly new to this and I think I just screwed something up when I did a rebase. Trying to fix…

@emilyaviva
Copy link
Contributor Author

Ok, how's that?

@Trott
Copy link
Member

Trott commented Oct 29, 2015

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.

@silverwind
Copy link
Contributor

LGTM

@Fishrock123
Copy link
Contributor

Double spaces don't actually render in HTML as far as I know?

@emilyaviva
Copy link
Contributor Author

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.

@Trott
Copy link
Member

Trott commented Oct 30, 2015

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.

@thefourtheye
Copy link
Contributor

Okay, as this patch looks okay to other collaborators, I'll not stand in the way of this doc-only change. LGTM.

@Fishrock123
Copy link
Contributor

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.

Anyway, the final '.' is added here anyway too.

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.

Trott pushed a commit that referenced this pull request Oct 31, 2015
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>
@Trott
Copy link
Member

Trott commented Oct 31, 2015

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.

@Trott Trott closed this Oct 31, 2015
rvagg pushed a commit that referenced this pull request Nov 7, 2015
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>
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Dec 29, 2015
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>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
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>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants