os: make EOL configurable and readonly by XadillaX · Pull Request #14622 · nodejs/node

XadillaX · 2017-08-04T06:28:12Z

Checklist

make -j4 test (UNIX)
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

os

richardlau · 2017-08-04T10:09:45Z

+
+const eol = common.isWindows ? '\r\n' : '\n';
+
+assert.strictEqual(eol, os.EOL);


Assertion parameters should be actual, expected so I think these should be the other way around.

tniessen

LGTM with @richardlau's comment addressed.

tniessen · 2017-08-04T11:33:03Z

+
+assert.strictEqual(eol, os.EOL);
+
+common.expectsError(function() {


I know that we didn't reach consensus on this, but I personally prefer arrow functions in our tests to make them a little more readable.

IMHO I'd like to use anonymous function.

Refs: #14496 (comment)

Even though there was no result about that discussion.

refack · 2017-08-04T14:59:10Z

Optional

@XadillaX we could take this one step further. Move EOL to node_contants.c defined with NODE_DEFINE_CONSTANT. Then the os.EOL property would be just a getter {get: () => constants.EOL} (maybe even deprecate it, now or later).

jasnell · 2017-08-04T15:02:46Z

@ChALkeR ... any way we can get an estimate on any breakage this may cause? (if any)

TimothyGu · 2017-08-04T15:51:21Z

@refack Those constants are all numbers, used as flags to be passed between C++ and JS. I don't see a reason to introduce an inconsistency here.

TimothyGu · 2017-08-04T15:53:04Z

+  },
+
+  EOL: {
+    configurable: false,


On a second thought, maybe making it configurable would be better? This way people who really need to change it for testing can do so, but requiring them to explicitly state their intentions. I'm okay either way though.

addaleax

I think @TimothyGu’s comment is a blocker.

(Also, I’m kind of missing the motivation here… people don’t accidentally override os.EOL, do they?)

richardlau · 2017-08-04T16:54:14Z

(Also, I’m kind of missing the motivation here… people don’t accidentally override os.EOL, do they?)

From #14619:

os.EOL is specified as a constant but its value is getting updated

So an alternative is to update the docs.

tniessen · 2017-08-04T17:06:13Z

people don’t accidentally override os.EOL, do they?

@addaleax In my opinion, this should throw when in strict mode:

if(os.EOL = 'foo') {
  console.log('Nope');
}

I am ±0 about making it configurable, but it should be a constant by default.

TimothyGu

Still LGTM, just even more so!

XadillaX · 2017-08-07T03:11:12Z

@addaleax How's the code now?

outdated

addaleax · 2017-08-07T13:07:52Z

I’ve removed my Changes Requested label, but as I mentioned I don’t quite see the point here.

Maybe it’s a miscommunication about what “constant” means in this context; if documentation for JS code says that foo.bar is a constant, I personally would expect that to mean that the value is not changed by whatever code provides that value, i.e. in this specific case that Node doesn’t change that value while a process is running.

ghost · 2017-08-07T22:16:32Z

@jasnell did a grep on Gzemnid dump from 2017-02-23, no assignment to EOL found.
@XadillaX added a test case for configurability in 3e31ac4, feel free to push out or edit.

ghost · 2017-08-07T22:17:37Z

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

ghost · 2017-08-08T22:57:47Z

Marking this one ctc-review. @nodejs/ctc ... please weigh in

ghost · 2017-08-11T02:32:50Z

@jasnell, @joyeecheung just reviewed

ghost

LGTM with good CI and CITGM run.

ghost

¯\_(ツ)_/¯

ghost · 2017-08-14T18:08:25Z

-0. In general I'm not sure it's a good idea to make properties like this non-configurable, because it removes an escape hatch. I can imagine a hypothetical test helper to verify an that application works on multiple platforms, which could rely on mutating os.EOL. We don't have any evidence that the current behavior has caused problems in practice, so changing it seems like it's a solution without a problem.

If we're convinced that we want to make the property non-writable to avoid accidental mutation, could we keep the property configurable? That way it would still be possible to overwrite the property as an escape hatch, but users would be unlikely to mutate it by mistake.

edit: I just saw that it is configurable the current version of the PR. The title of the PR still says it's non-configurable.

ghost

LGTM with { configurable: true }

ghost · 2017-08-14T18:13:47Z

CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/951/

I count 4 ✔️s from CTC members, so IMHO this is ready to land (pending CitGM)

ghost · 2017-08-16T09:44:10Z

before landing, can this get a rebase? We are getting some failures for body-parser that are already fixed on master.

ghost

LGTM if CITGM is ok

ghost · 2017-08-17T03:54:02Z

@mcollina rebased.

ghost · 2017-08-17T07:34:14Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/957/

ghost · 2017-08-17T07:43:26Z

Reminder to whoever lands this: the commit message should be updated to correct "non-configurable" to "configurable".

Refs: nodejs#14619

ghost · 2017-08-17T09:50:11Z

@not-an-aardvark Thanks and I've rebased the message.

ghost · 2017-08-17T10:36:38Z

I think CITGM is green, this can be landed.

ghost · 2017-08-17T12:54:42Z

@XadillaX regarding the commit message, I think the patch "Fixes:", not just "Refs:" the issue, doesn't it?

ghost · 2017-08-21T02:51:18Z

@aqrln Right, I think who to land this may help me to modify the commit message.

ghost · 2017-08-21T03:12:36Z

Pre-land CI: https://ci.nodejs.org/job/node-test-commit/11929/

ghost · 2017-08-21T04:24:19Z

Landed in f6caeb9

nodejs-github-bot added the os Issues and PRs related to the os subsystem. label Aug 4, 2017

XadillaX added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 4, 2017

vsemozhetbyt approved these changes Aug 4, 2017

View reviewed changes

TimothyGu approved these changes Aug 4, 2017

View reviewed changes

richardlau reviewed Aug 4, 2017

View reviewed changes

tniessen approved these changes Aug 4, 2017

View reviewed changes

cjihrig approved these changes Aug 4, 2017

View reviewed changes

lpinca approved these changes Aug 4, 2017

View reviewed changes

TimothyGu approved these changes Aug 4, 2017

View reviewed changes

addaleax previously requested changes Aug 4, 2017

View reviewed changes

TimothyGu approved these changes Aug 6, 2017

View reviewed changes

aqrln approved these changes Aug 6, 2017

View reviewed changes

jasnell added the ctc-review label Aug 8, 2017

joyeecheung approved these changes Aug 10, 2017

View reviewed changes

jasnell approved these changes Aug 11, 2017

View reviewed changes

Trott mentioned this pull request Aug 13, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-08-16 nodejs/CTC#162

Closed

rvagg approved these changes Aug 14, 2017

View reviewed changes

Fishrock123 reviewed Aug 14, 2017

View reviewed changes

refack approved these changes Aug 14, 2017

View reviewed changes

aqrln changed the title ~~os: make EOL non-configurable and readonly~~ os: make EOL configurable and readonly Aug 15, 2017

mcollina approved these changes Aug 16, 2017

View reviewed changes

XadillaX force-pushed the feature/os-eol branch from 3e31ac4 to 042c259 Compare August 17, 2017 03:53

XadillaX changed the title ~~os: make EOL configurable and readonly~~ os: make EOL non-configurable and readonly Aug 17, 2017

XadillaX changed the title ~~os: make EOL non-configurable and readonly~~ os: make EOL configurable and readonly Aug 17, 2017

os: make EOL configurable and readonly

6f5087d

Refs: nodejs#14619

XadillaX force-pushed the feature/os-eol branch from 042c259 to 6f5087d Compare August 17, 2017 09:49

refack closed this Aug 21, 2017

Trott removed the ctc-review label Aug 21, 2017

jasnell mentioned this pull request Oct 30, 2017

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

Merged


		const eol = common.isWindows ? '\r\n' : '\n';

		assert.strictEqual(eol, os.EOL);


		assert.strictEqual(eol, os.EOL);

		common.expectsError(function() {

Uh oh!

Conversation

XadillaX commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

richardlau Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

XadillaX Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Optional

Uh oh!

jasnell commented Aug 4, 2017

Uh oh!

TimothyGu commented Aug 4, 2017

Uh oh!

TimothyGu Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

XadillaX Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau commented Aug 4, 2017

Uh oh!

tniessen commented Aug 4, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

XadillaX commented Aug 7, 2017

Uh oh!

addaleax commented Aug 7, 2017

Uh oh!

ghost commented Aug 7, 2017

Uh oh!

ghost commented Aug 7, 2017

Uh oh!

ghost commented Aug 8, 2017

Uh oh!

ghost commented Aug 11, 2017

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 14, 2017 • edited by not-an-aardvark Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 14, 2017

Uh oh!

ghost commented Aug 16, 2017

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 17, 2017

Uh oh!

ghost commented Aug 17, 2017

Uh oh!

ghost commented Aug 17, 2017

Uh oh!

XadillaX commented Aug 4, 2017 •

edited

Loading

refack commented Aug 4, 2017 •

edited

Loading

ghost commented Aug 14, 2017 •

edited by not-an-aardvark

Loading