tools: modernize tools/license2rtf.js by tniessen · Pull Request #16220 · nodejs/node

tniessen · 2017-10-15T15:24:24Z

This PR tries to modernize some tooling code. Should still produce the same result:

$ git checkout master
Switched to branch 'master'
$ Release/node tools/license2rtf.js > LICENSE_ORIGINAL.rtf
$ cat LICENSE | Release/node tools/license2rtf.js > LICENSE_ORIGINAL.rtf
$ git checkout tools-license-es6
Switched to branch 'tools-license-es6'
$ cat LICENSE | Release/node tools/license2rtf.js > LICENSE_NEW.rtf
$ sha256sum LICENSE_ORIGINAL.rtf LICENSE_NEW.rtf
4331ecea25a7cfc6948d7fdd8b3e298f54de913591e6a3962b8302b80179acbb *LICENSE_ORIGINAL.rtf
4331ecea25a7cfc6948d7fdd8b3e298f54de913591e6a3962b8302b80179acbb *LICENSE_NEW.rtf

Changes:

ES classes instead of util.inherits
ES string functions (trimRight, padStart)
Template strings

There are still some possible improvements but it's a start.

Checklist

commit message follows commit guidelines

Affected core subsystem(s)

tools

TimothyGu · 2017-10-15T16:32:36Z

tools/license2rtf.js

-
-  Stream.call(this);
-  this.writable = true;
+class LineSplitter extends Stream {


Why not inherit from Stream.Writable which provides automatic backpressure?

Good point, I mostly just rewrote the classes without changing any semantics / inheritance.

lpinca

SGTM.

lpinca · 2017-10-16T07:54:09Z

tools/license2rtf.js


-  function rtfEscape(string) {
+  rtfEscape(string) {
+    function toHex(number, length) {


Nit: I would move this to the top scope.

targos · 2017-10-28T13:45:14Z

ping @tniessen

tniessen · 2017-10-28T15:11:49Z

I would like to land this without addressing #16220 (comment) which can be done in a separate PR (unless someone objects).

tniessen · 2017-10-29T18:48:39Z

Landed in 9d76850, e2ce130.

PR-URL: #16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs/node#16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

MylesBorins · 2017-11-16T21:31:38Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

PR-URL: nodejs/node#16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

tniessen added 2 commits October 15, 2017 14:32

tools: replace loop with padStart

65eb726

tools: modernize license2rtf

70682b3

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 15, 2017

tniessen added the refactor to ES6+ label Oct 15, 2017

tniessen self-assigned this Oct 15, 2017

TimothyGu reviewed Oct 15, 2017

View reviewed changes

lpinca approved these changes Oct 16, 2017

View reviewed changes

targos approved these changes Oct 22, 2017

View reviewed changes

jasnell approved these changes Oct 22, 2017

View reviewed changes

[fixup] lpinca's nit

a26186d

tniessen closed this Oct 29, 2017

tniessen added a commit that referenced this pull request Oct 29, 2017

tools: replace loop with padStart

9d76850

PR-URL: #16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

tniessen added a commit that referenced this pull request Oct 29, 2017

tools: modernize license2rtf

e2ce130

PR-URL: #16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn pushed a commit that referenced this pull request Oct 30, 2017

tools: replace loop with padStart

8c1332c

PR-URL: #16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

gibfahn pushed a commit that referenced this pull request Oct 30, 2017

tools: modernize license2rtf

d079a81

PR-URL: #16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

gibfahn pushed a commit that referenced this pull request Oct 30, 2017

tools: replace loop with padStart

e402694

PR-URL: #16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

gibfahn pushed a commit that referenced this pull request Oct 30, 2017

tools: modernize license2rtf

473377f

PR-URL: #16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

gibfahn pushed a commit that referenced this pull request Oct 31, 2017

tools: replace loop with padStart

d5891b5

PR-URL: #16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

gibfahn pushed a commit that referenced this pull request Oct 31, 2017

tools: modernize license2rtf

3d8a7e9

PR-URL: #16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017

tools: modernize license2rtf

07fa7b7

PR-URL: nodejs/node#16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017

tools: modernize license2rtf

d1dbd95

PR-URL: nodejs/node#16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

tools: modernize license2rtf

edd9d35

PR-URL: nodejs/node#16220 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

tools: modernize tools/license2rtf.js#16220

tools: modernize tools/license2rtf.js#16220
tniessen wants to merge 3 commits intonodejs:masterfrom
tniessen:tools-license-es6

tniessen commented Oct 15, 2017

Uh oh!

TimothyGu Oct 15, 2017

Uh oh!

tniessen Oct 15, 2017

Uh oh!

lpinca left a comment

Uh oh!

lpinca Oct 16, 2017

Uh oh!

targos commented Oct 28, 2017

Uh oh!

tniessen commented Oct 28, 2017

Uh oh!

tniessen commented Oct 29, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

tniessen commented Oct 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

TimothyGu Oct 15, 2017

Choose a reason for hiding this comment

Uh oh!

tniessen Oct 15, 2017

Choose a reason for hiding this comment

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca Oct 16, 2017

Choose a reason for hiding this comment

Uh oh!

targos commented Oct 28, 2017

Uh oh!

tniessen commented Oct 28, 2017

Uh oh!

tniessen commented Oct 29, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants