Correct typos #11189

bf4 · 2017-02-06T01:20:31Z

go get -u github.com/client9/misspell/cmd/misspell
misspell  -w -error -source=text .

vkurchatkin · 2017-02-06T01:29:21Z

IMO this is not very good idea. Changes a bunch of comments, variable names, mostly in deps. At the very least you should exclude deps

cjihrig · 2017-02-06T01:44:37Z

Agreed. If you leave out the changes in deps/, this should be OK.

mscdex · 2017-02-06T01:57:22Z

I agree we should exclude deps/, but I think we should also more generally exclude any dependencies, no matter where they are located. That includes deps/ as well as various third-party dependencies in tools/ (e.g. tools/eslint and tools/gyp) for example.

bf4 · 2017-02-06T01:57:33Z

@cjihrig @vkurchatkin Sure, will update

bf4 · 2017-02-06T02:02:04Z

@mscdex @cjihrig @vkurchatkin Updated. Think I got the right surface area here? I removed any node_modules as well. (Of course I'll rebase)

mscdex · 2017-02-06T02:05:34Z

@bf4 I think the tools/eslint, tools/gyp, and tools/icu changes should be reverted as well as those changes are all to upstream files.

bf4 · 2017-02-06T02:12:06Z

@mscdex Added another commit removing anything from tools for ease of review

mscdex · 2017-02-06T02:19:39Z

src/node_lttng_provider.h

@@ -78,7 +78,7 @@ void NODE_GC_START(v8::GCType type,
 void NODE_GC_DONE(v8::GCType type,
                  v8::GCCallbackFlags flags,
                  v8::Isolate* isolate) {
-  const char* typeStr = "Unkown GC Type";
+  const char* typeStr = "Unknown GC Type";


@nodejs/ctc Does this constitute a semver-major change?

funny what spell checkers find :) would this be considered a public API...

Well it's a tracing mechanism, which may/may not be considered part of the "runtime." That's why I'd like to get some feedback to see where this kind of change would fall since AFAIK error string changes in general are still semver-major changes.

@nodejs/ctc What do you think about this error message change as far as semver-ness goes?

If this is an officially supported feature, then semver major. Honestly, version 8 is coming up soon, so maybe just make it semver major anyway.

mscdex · 2017-02-06T02:20:42Z

doc/changelogs/CHANGELOG_V6.md

@@ -1362,7 +1362,7 @@ Semver Patch:
 * [[`abf86adee1`](https://github.com/nodejs/node/commit/abf86adee1)] - **deps**: back-port d721121 from v8 upstream (Ben Noordhuis) [#7633](https://github.com/nodejs/node/pull/7633)
 * [[`dbdcded866`](https://github.com/nodejs/node/commit/dbdcded866)] - **deps**: upgrade to V8 5.0.71.54 (Ben Noordhuis) [#7531](https://github.com/nodejs/node/pull/7531)
 * [[`4839ef37a9`](https://github.com/nodejs/node/commit/4839ef37a9)] - **doc**: correcting misspelling (Vitaly Tomilov) [#7797](https://github.com/nodejs/node/pull/7797)
-* [[`3343d46f2c`](https://github.com/nodejs/node/commit/3343d46f2c)] - **doc**: general improvments to events documentation (Sakthipriyan Vairamani) [#7480](https://github.com/nodejs/node/pull/7480)
+* [[`3343d46f2c`](https://github.com/nodejs/node/commit/3343d46f2c)] - **doc**: general improvements to events documentation (Sakthipriyan Vairamani) [#7480](https://github.com/nodejs/node/pull/7480)


I'm not sure this change should be kept, it's referencing a commit message which would still have the original misspelling and that might cause confusion.

I agree, let's not fix the commit messages in changelogs.

mscdex · 2017-02-06T02:21:39Z

doc/changelogs/CHANGELOG_V4.md

@@ -813,7 +813,7 @@ Semver Patch:
 * [[`1e86d16812`](https://github.com/nodejs/node/commit/1e86d16812)] - **doc**: buffers are not sent over IPC with a socket (Tim Kuijsten) [#6951](https://github.com/nodejs/node/pull/6951)
 * [[`5c1d8e1f0f`](https://github.com/nodejs/node/commit/5c1d8e1f0f)] - **doc**: add `added:` information for http (Anna Henningsen) [#7392](https://github.com/nodejs/node/pull/7392)
 * [[`60c054bc11`](https://github.com/nodejs/node/commit/60c054bc11)] - **doc**: add information for IncomingMessage.destroy() (Rich Trott) [#7237](https://github.com/nodejs/node/pull/7237)
-* [[`1a5c025f32`](https://github.com/nodejs/node/commit/1a5c025f32)] - **doc**: remove superfluos backticks in process.md (Anna Henningsen) [#7681](https://github.com/nodejs/node/pull/7681)
+* [[`1a5c025f32`](https://github.com/nodejs/node/commit/1a5c025f32)] - **doc**: remove superfluous backticks in process.md (Anna Henningsen) [#7681](https://github.com/nodejs/node/pull/7681)


Ditto about all of these changes to commit messages in the changelogs.

mscdex · 2017-02-06T02:22:37Z

doc/changelogs/CHANGELOG_V5.md

@@ -215,7 +215,7 @@ This is a security release. All Node.js users should consult the security releas
 ### Notable changes

 **http**:
-  * Enclose IPv6 Host header in square brackets. This will enable proper separation of the host adress from any port reference (Mihai Potra) [#5314](https://github.com/nodejs/node/pull/5314)
+  * Enclose IPv6 Host header in square brackets. This will enable proper separation of the host address from any port reference (Mihai Potra) [#5314](https://github.com/nodejs/node/pull/5314)


This change should be fine I think since it was actually spelled correctly in the relevant commit message.

mscdex · 2017-02-06T02:23:21Z

A few nits and a question, otherwise the rest LGTM though.

bf4 · 2017-02-06T02:30:08Z

@mscdex Thanks for your review. I'll wait on getting confirmation of how to handle the changelogs before making more changes, then I'll push changes and squash down to one commit.

bf4 · 2017-02-06T17:22:33Z

@mscdex @thefourtheye should I wait for any more comments before updating?

mscdex · 2017-02-06T17:28:15Z

@bf4 Feel free to update, we're just waiting on more input from other collaborators on the change in the one error message string.

bf4 · 2017-02-22T20:40:56Z

@mscdex @cjihrig I've updated the PR to add [sic] for typos we're quoting from other sources, and moved the breaking changed into a code comment.

--- a/src/node_lttng_provider.h
+++ b/src/node_lttng_provider.h
@@ -61,7 +61,7 @@ void NODE_NET_STREAM_END(node_lttng_connection_t* conn,
 void NODE_GC_START(v8::GCType type,
                    v8::GCCallbackFlags flags,
                    v8::Isolate* isolate) {
-  const char* typeStr = "Unknown GC Type";
+  const char* typeStr = "Unkown GC Type"; // [sic]
   const char* flagsStr = "Unknown GC Flag";

 #define BUILD_IF(f) if (type == v8::GCType::f) { typeStr = #f; }
@@ -78,7 +78,7 @@ void NODE_GC_START(v8::GCType type,
 void NODE_GC_DONE(v8::GCType type,
                   v8::GCCallbackFlags flags,
                   v8::Isolate* isolate) {
-  const char* typeStr = "Unknown GC Type";
+  const char* typeStr = "Unkown GC Type"; // [sic]
   const char* flagsStr = "Unknown GC Flag";

mscdex · 2017-02-23T03:54:37Z

@bf4 There's a typo in node.cc that you might want to include (s/enought/enough/).

bf4 · 2017-02-23T21:24:22Z

@mscdex Pushed. would you like me to rebase and squash down to one commit?

mscdex · 2017-02-23T22:35:33Z

@bf4 Nah, that can be done when landing. I plan on landing this tonight if nobody else beats me to it.

mhdawson

LGTM

jasnell · 2017-02-25T01:54:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6581/

bf4 · 2017-02-27T04:56:38Z

@jasnell If I'm reading that correctly, there's a conflict from an earlier commit in the PR? CONFLICT (modify/delete): deps/v8/src/base/atomicops_internals_arm_gcc.h deleted in HEAD and modified in Correct typos. Version Correct typos of deps/v8/src/base/atomicops_internals_arm_gcc.h left in tree. Would a rebase address that?

mscdex · 2017-02-28T03:33:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6620/

mscdex · 2017-02-28T03:35:20Z

doc/changelogs/CHANGELOG_IOJS.md

@@ -2207,7 +2207,7 @@ _Note: version **1.4.0** was tagged and built but not released. A libuv bug was
 * [[`3b1b4de903`](https://github.com/nodejs/node/commit/3b1b4de903)] - **test**: Timeout#unref() does not return instance (Jan Schär) [joyent/node#9171](https://github.com/joyent/node/pull/9171)
 * [[`becb4e980e`](https://github.com/nodejs/node/commit/becb4e980e)] - **test**: distribute crypto tests into separate files (Brendan Ashworth) [#827](https://github.com/nodejs/node/pull/827)
 * [[`77f35861d0`](https://github.com/nodejs/node/commit/77f35861d0)] - **(SEMVER-MINOR) tls**: more secure defaults (Roman Reiss) [#826](https://github.com/nodejs/node/pull/826)
-* [[`faa687b4be`](https://github.com/nodejs/node/commit/faa687b4be)] - **url**: reslove urls with . and .. (Amir Saboury) [#278](https://github.com/nodejs/node/pull/278)
+* [[`faa687b4be`](https://github.com/nodejs/node/commit/faa687b4be)] - **url**: reslove [sic] urls with . and .. (Amir Saboury) [#278](https://github.com/nodejs/node/pull/278)


I'm not sure if adding '[sic]' helps in the commit messages in changelogs since the way the commits are laid out it could be seen as being part of the literal commit message.

mscdex · 2017-02-28T03:37:33Z

@bf4 It looks like the linter is complaining.

mscdex · 2017-02-28T04:26:39Z

doc/changelogs/CHANGELOG_IOJS.md

@@ -2494,7 +2494,7 @@ _Note: version **1.4.0** was tagged and built but not released. A libuv bug was
 * [[`b7365c1`](https://github.com/nodejs/node/commit/b7365c15597253e906590045aa6f3f07f6e76b52)] - repl: make REPL support multiline template literals (Xiaowei Li)
 * [[`2253d30`](https://github.com/nodejs/node/commit/2253d30d9cbba42abc1faa183e4480cac69c4222)] - build: remove unused variable (Johan Bergström)
 * [[`ab04a43`](https://github.com/nodejs/node/commit/ab04a434761cf66d107481d58798f36d3cb49d46)] - doc: add optional sudo to make install in README (Glen Keane)
-* [[`1b1cd1c`](https://github.com/nodejs/node/commit/1b1cd1c3f8e21b34a8e1355e545057a661acaa15)] - build: shorten configurate script print on stdout (Roman Reiss)
+* [[`1b1cd1c`](https://github.com/nodejs/node/commit/1b1cd1c3f8e21b34a8e1355e545057a661acaa15)] - build: shorten configurate [sic] script print on stdout (Roman Reiss)


This one should probably be removed also.

oops, that's embarassing

mscdex · 2017-02-28T15:18:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6626/

bf4 · 2017-02-28T22:52:29Z

I'm finding it unusually difficult to find the actual failure in the CI logs.

``` go get -u github.com/client9/misspell/cmd/misspell misspell -w -error -source=text . ```

bf4 · 2017-02-28T22:59:38Z

I rebased off of master to adjust for the conflicting file that was moved 3c92200

mscdex · 2017-02-28T23:38:17Z

@bf4 There weren't any failures in the last CI run. However, here is another run with your recent change: https://ci.nodejs.org/job/node-test-pull-request/6632/

bf4 · 2017-03-01T20:47:40Z

src/string_search.h

@@ -238,7 +238,7 @@ inline const void* MemrchrFill(const void* haystack, uint8_t needle,
 }


-// Finds the first occurence of *two-byte* character pattern[0] in the string
+// Finds the first occurrence of *two-byte* character pattern[0] in the string


I think this file comes from deps/v8/src/string-search.h with the typo ?

bf4 · 2017-03-01T20:53:24Z

@mscdex I don't see how those failure could be related to this PR

When I drill down

https://ci.nodejs.org/job/node-test-binary-windows/6806/RUN_SUBSET=1,VS_VERSION=vs2015,label=win2008r2/tapResults/ relates to a file lib/_debugger.js not in this diff

// parallel/test-debug-usage	

assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: 'Usage: node debug script.js\n' === 'Usage: node debug script.js\n       node debug <host>:<port>\n       node debug -p <pid>\n'
    at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2015\label\win2008r2\test\parallel\test-debug-usage.js:20:10)
    at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2015\label\win2008r2\test\common.js:452:15)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:194:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:208:12)

and the other https://ci.nodejs.org/job/node-test-commit-arm/8016/nodes=armv8-ubuntu1404/console relates to a failure to publish TAP results

ERROR: Step ‘Publish TAP Results’ failed: no workspace for node-test-commit-arm/nodes=armv8-ubuntu1404 #8016
Checking ^not ok
ERROR: Build step failed with exception
java.lang.NullPointerException
	at hudson.plugins.textfinder.TextFinderPublisher.findText(TextFinderPublisher.java:101)
	at hudson.plugins.textfinder.TextFinderPublisher.perform(TextFinderPublisher.java:74)
	at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
	at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
	at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:720)
	at hudson.model.Build$BuildExecution.post2(Build.java:185)
	at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:665)
	at hudson.model.Run.execute(Run.java:1753)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:404)
Build step 'Jenkins Text Finder' marked build as failure
Notifying upstream projects of job completion
Finished: FAILURE

I do wonder if src/string-search.h should be included as I think it's coming from deps/v8/src/string-search.h but they're not the same

mscdex · 2017-03-01T20:53:51Z

@bf4 You're correct, they're unrelated. The changes should be good to go now.

bf4 · 2017-03-03T18:40:36Z

Lemme know if there's anything else for me to do.

mscdex · 2017-03-03T21:29:58Z

Landed in 4897ae2. Thanks!

PR-URL: #11189 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Is a semver-breaking change Refs: nodejs#11189 (comment)

Is a semver-breaking change Refs: #11189 (comment) PR-URL: #11723 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Is a semver-breaking change Refs: nodejs#11189 (comment) PR-URL: nodejs#11723 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2017-04-17T21:41:31Z

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

nodejs-github-bot added build Issues and PRs related to build files or the CI. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. labels Feb 6, 2017

bf4 mentioned this pull request Feb 6, 2017

Fix typos nodejs/nodejs.org#1130

Merged

mscdex reviewed Feb 6, 2017

View reviewed changes

mscdex mentioned this pull request Feb 10, 2017

benchmark: fix typos #11287

Merged

2 tasks

bf4 force-pushed the correct_spelling branch 2 times, most recently from 774efc1 to c1cea32 Compare February 22, 2017 20:46

jasnell approved these changes Feb 24, 2017

View reviewed changes

mhdawson approved these changes Feb 24, 2017

View reviewed changes

mscdex reviewed Feb 28, 2017

View reviewed changes

bf4 force-pushed the correct_spelling branch from 0f8ebaa to 8b90964 Compare February 28, 2017 15:12

thefourtheye approved these changes Feb 28, 2017

View reviewed changes

bf4 added 2 commits February 28, 2017 16:58

Correct typos

c6b27bc

``` go get -u github.com/client9/misspell/cmd/misspell misspell -w -error -source=text . ```

Remove [sic] annotations to known typos

1b4a66b

bf4 force-pushed the correct_spelling branch from 8b90964 to 1b4a66b Compare February 28, 2017 22:58

bf4 commented Mar 1, 2017

View reviewed changes

mscdex closed this Mar 3, 2017

bf4 deleted the correct_spelling branch March 6, 2017 22:18

bf4 mentioned this pull request Mar 6, 2017

src: fix typos in node_lttng_provider.h #11723

Closed

bf4 added a commit to bf4/node that referenced this pull request Mar 7, 2017

src: fix typos in node_lttng_provider.h

cfe7339

Is a semver-breaking change Refs: nodejs#11189 (comment)

evanlucas mentioned this pull request Mar 8, 2017

v7.7.2 proposal #11745

Merged

MylesBorins added the backport-requested-v6.x label Apr 17, 2017

Correct typos #11189

Correct typos #11189

Conversation

bf4 commented Feb 6, 2017

vkurchatkin commented Feb 6, 2017

cjihrig commented Feb 6, 2017

mscdex commented Feb 6, 2017 • edited Loading

bf4 commented Feb 6, 2017

bf4 commented Feb 6, 2017 • edited Loading

mscdex commented Feb 6, 2017

bf4 commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex commented Feb 6, 2017 • edited Loading

bf4 commented Feb 6, 2017

bf4 commented Feb 6, 2017

mscdex commented Feb 6, 2017

bf4 commented Feb 22, 2017

mscdex commented Feb 23, 2017

bf4 commented Feb 23, 2017

mscdex commented Feb 23, 2017

mhdawson left a comment

Choose a reason for hiding this comment

jasnell commented Feb 25, 2017

bf4 commented Feb 27, 2017

mscdex commented Feb 28, 2017

Choose a reason for hiding this comment

mscdex commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Feb 28, 2017 • edited Loading

bf4 commented Feb 28, 2017

bf4 commented Feb 28, 2017

mscdex commented Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Mar 1, 2017 • edited Loading

mscdex commented Mar 1, 2017

bf4 commented Mar 3, 2017

mscdex commented Mar 3, 2017

MylesBorins commented Apr 17, 2017

mscdex commented Feb 6, 2017 •

edited

Loading

bf4 commented Feb 6, 2017 •

edited

Loading

mscdex Feb 6, 2017 •

edited

Loading

mscdex Feb 6, 2017 •

edited

Loading

mscdex commented Feb 6, 2017 •

edited

Loading

mscdex commented Feb 28, 2017 •

edited

Loading

mscdex commented Feb 28, 2017 •

edited

Loading

bf4 commented Mar 1, 2017 •

edited

Loading