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

Correct typos #11189

Closed
wants to merge 2 commits into from
Closed

Correct typos #11189

wants to merge 2 commits into from

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Feb 6, 2017

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

@nodejs-github-bot 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
@vkurchatkin
Copy link
Contributor

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
Copy link
Contributor

cjihrig commented Feb 6, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Feb 6, 2017

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
Copy link
Contributor Author

bf4 commented Feb 6, 2017

@cjihrig @vkurchatkin Sure, will update

@bf4
Copy link
Contributor Author

bf4 commented Feb 6, 2017

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

@bf4 bf4 mentioned this pull request Feb 6, 2017
@mscdex
Copy link
Contributor

mscdex commented Feb 6, 2017

@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
Copy link
Contributor Author

bf4 commented Feb 6, 2017

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

@@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mscdex mscdex Feb 6, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Contributor

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.

@@ -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)
Copy link
Contributor

@mscdex mscdex Feb 6, 2017

Choose a reason for hiding this comment

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

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

@mscdex
Copy link
Contributor

mscdex commented Feb 6, 2017

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

@bf4
Copy link
Contributor Author

bf4 commented Feb 6, 2017

@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
Copy link
Contributor Author

bf4 commented Feb 6, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Feb 6, 2017

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

@mscdex mscdex mentioned this pull request Feb 10, 2017
2 tasks
@bf4
Copy link
Contributor Author

bf4 commented Feb 22, 2017

@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";

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

mscdex commented Feb 23, 2017

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

@bf4
Copy link
Contributor Author

bf4 commented Feb 23, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Feb 23, 2017

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell
Copy link
Member

jasnell commented Feb 25, 2017

@bf4
Copy link
Contributor Author

bf4 commented Feb 27, 2017

@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
Copy link
Contributor

mscdex commented Feb 28, 2017

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

mscdex commented Feb 28, 2017

@bf4 It looks like the linter is complaining.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should probably be removed also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, that's embarassing

@mscdex
Copy link
Contributor

mscdex commented Feb 28, 2017

@bf4
Copy link
Contributor Author

bf4 commented Feb 28, 2017

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
Copy link
Contributor Author

bf4 commented Feb 28, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Feb 28, 2017

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

@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That's ok.

@bf4
Copy link
Contributor Author

bf4 commented Mar 1, 2017

@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
Copy link
Contributor

mscdex commented Mar 1, 2017

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

@bf4
Copy link
Contributor Author

bf4 commented Mar 3, 2017

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

@mscdex
Copy link
Contributor

mscdex commented Mar 3, 2017

Landed in 4897ae2. Thanks!

@mscdex mscdex closed this Mar 3, 2017
mscdex pushed a commit that referenced this pull request Mar 3, 2017
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>
addaleax pushed a commit that referenced this pull request Mar 5, 2017
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>
@bf4 bf4 deleted the correct_spelling branch March 6, 2017 22:18
bf4 added a commit to bf4/node that referenced this pull request Mar 7, 2017
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
addaleax pushed a commit that referenced this pull request Mar 10, 2017
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>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants