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

doc: add missing periods in documentation.md #20469

Closed
wants to merge 1 commit into from
Closed

doc: add missing periods in documentation.md #20469

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented May 2, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Our MD-2-HTML converter replaces line breaks with spaces in these fragments so the result becomes less readable after that without periods.

Before this PR:

s1

After this PR:

s2

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label May 2, 2018
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 2, 2018
@vsemozhetbyt
Copy link
Contributor Author

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

@vsemozhetbyt
Copy link
Contributor Author

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it will be rendered without a line break, I wonder if it makes sense to remove the line break in the .md file too? Otherwise, what's to stop someone (probably me!) coming along in 6 months and opening a fast-track pull request to remove these "unnecessary" periods? :-D

@vsemozhetbyt
Copy link
Contributor Author

Amended.
@Trott, @tniessen, is this still LG to you?
CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/651/

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistically, I prefer the previous version with a line break, but @Trott's argumentation is plausible.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate PR for sure, but I would be all for shortening these blobs of text. For example, the first one would probably be more readable and effective like this:

Stability: 0 - Deprecated. Use of this feature may cause warnings to be emitted.
Backwards compatibility across major versions should not be expected.

(The removed text just basically says what it is already adequately communicated by the word "Deprecated".)

That sort of change is likely to generate bike-shedding, though, so definitely separate PR. :-D

@vsemozhetbyt
Copy link
Contributor Author

Landed in 2553377
Thanks for the reviews.

@vsemozhetbyt vsemozhetbyt deleted the doc-doc-stability-periods branch May 3, 2018 09:14
vsemozhetbyt added a commit that referenced this pull request May 3, 2018
PR-URL: #20469
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 4, 2018
PR-URL: #20469
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
MylesBorins pushed a commit that referenced this pull request May 8, 2018
PR-URL: #20469
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants