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: grammar fix in dns docs #44850

Merged
merged 1 commit into from Oct 4, 2022
Merged

doc: grammar fix in dns docs #44850

merged 1 commit into from Oct 4, 2022

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 1, 2022

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations. labels Oct 1, 2022
@lpinca
Copy link
Member

lpinca commented Oct 1, 2022

Swap "grammar" and "fix" in commit message?

doc/api/dns.md Outdated Show resolved Hide resolved
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.

I know we don't always enforce it and I'm not sure we even need it, but our commit message guidelines say the first word after the subsystem should be an imperative mood verb. So, as @lpinca suggests, "fix grammar in dns docs" would be better than "grammar fix in dns docs".

If you want to get really nit-picky, I'm not sure the change here is technically a grammar fix. It's more of a style change? But now I'm getting into really low-value comments and I should stop myself.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 1, 2022
@Trott Trott added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 2, 2022
Also removes some italics text, as requested
in the pull request review.

PR-URL: #44850
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@cjihrig cjihrig merged commit 8d5eef5 into main Oct 4, 2022
@cjihrig cjihrig deleted the cjihrig-patch-1 branch October 4, 2022 13:09
danielleadams pushed a commit that referenced this pull request Oct 11, 2022
Also removes some italics text, as requested
in the pull request review.

PR-URL: #44850
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dns Issues and PRs related to the dns subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants