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

@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

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

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

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

doc/changelogs/CHANGELOG_V5.md

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

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

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



-// 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Is a semver-breaking change Refs: nodejs#11189 (comment) PR-URL: nodejs#11723 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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

doc/changelogs/CHANGELOG_V4.md Outdated

Copy link

Contributor

mscdex Feb 6, 2017

Choose a reason for hiding this comment

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

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

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



		// 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

Uh oh!

Correct typos #11189

Correct typos #11189

Uh oh!

Conversation

bf4 commented Feb 6, 2017

Uh oh!

vkurchatkin commented Feb 6, 2017

Uh oh!

cjihrig commented Feb 6, 2017

Uh oh!

mscdex commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bf4 commented Feb 6, 2017

Uh oh!

bf4 commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Feb 6, 2017

Uh oh!

bf4 commented Feb 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bf4 commented Feb 6, 2017

Uh oh!

bf4 commented Feb 6, 2017

Uh oh!

mscdex commented Feb 6, 2017

Uh oh!

bf4 commented Feb 22, 2017

Uh oh!

mscdex commented Feb 23, 2017

Uh oh!

bf4 commented Feb 23, 2017

Uh oh!

mscdex commented Feb 23, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 25, 2017

Uh oh!

bf4 commented Feb 27, 2017

Uh oh!

mscdex commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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