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

Compiler flag to specify line ending #2921

Merged
merged 9 commits into from May 4, 2015

Conversation

Projects
None yet
5 participants
@kmashint
Copy link
Contributor

commented Apr 26, 2015

This is a first pass, unit tests still TODO, of adding a compiler flag to specify the line ending:
#1693
--newLine NEWLINE Emit newline: 'CRLF' (dos) or 'LF' (unix).

I'll next add unit tests as noted in this follow-up issue but please note any comments on code thus far:
#2918

i've signed the contributor agreement.

@msftclas

This comment has been minimized.

Copy link

commented Apr 26, 2015

Hi @kmashint, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

@msftclas msftclas added the cla-signed label Apr 26, 2015

@DanielRosenwasser DanielRosenwasser changed the title Compiler flag to specify line ending #1693 Compiler flag to specify line ending Apr 26, 2015

"category": "Message",
"code": 6062
},
"Argument for --newLine option must be 'CRLF' or 'LF'.": {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Apr 26, 2015

Member

'--newLine' in single quotes.

@@ -1681,6 +1682,12 @@ module ts {
UMD = 3,
}

export const enum NewLineKind {
DEFAULT = 0,

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Apr 26, 2015

Member

Since each of the options are optional properties, I don't think we need a default.

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Apr 27, 2015

Contributor

Or, if we do, just call it 'Platform'.

@@ -91,6 +91,8 @@ module ts {
}
}

let newLine = [sys.newLine, "\r\n", "\n"][options.newLine ? Number(options.newLine) : 0];

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Apr 26, 2015

Member

This is kind of unclear at first sight. I might do

let newLine =
    options.newLine === NewLineKind.LF ? "\n" :
    options.newLine === NewLineKind.CRLF ? "\r\n" :
    sys.newLine;

This comment has been minimized.

Copy link
@CyrusNajmabadi
@@ -58,6 +58,13 @@ module ts {
error: Diagnostics.Argument_for_module_option_must_be_commonjs_amd_or_umd
},
{
name: "newLine",
type: { "crlf": NewLineKind.CRLF, "lf": NewLineKind.LF },

This comment has been minimized.

Copy link
@CyrusNajmabadi

CyrusNajmabadi Apr 27, 2015

Contributor

NewLineKind.LineFeed, .CarriageReturnLineFeed

@@ -1984,6 +1984,18 @@
"category": "Error",
"code": 6059
},
"Emit newline: 'CRLF' (dos) or 'LF' (unix).": {
"category": "Message",
"code": 6061

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Apr 27, 2015

Member

Why not 6060?

This comment has been minimized.

Copy link
@kmashint

kmashint Apr 27, 2015

Author Contributor

Sure, I'll adjust.

@@ -1650,6 +1650,7 @@ module ts {
locale?: string;
mapRoot?: string;
module?: ModuleKind;
newLine?: string;

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Apr 27, 2015

Member

This should be a NewLineKind.

@kmashint

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2015

Adjusted for comments thus far. I'll look into test cases tomorrow.

@@ -91,6 +91,11 @@ module ts {
}
}

let newLine =

This comment has been minimized.

Copy link
@mhegazy

mhegazy Apr 27, 2015

formatting, and use const.

@mhegazy

This comment has been minimized.

Copy link

commented Apr 27, 2015

Thanks @kmashint for the change. We will need a test for the new flag. to add a test,

  • add a new test file in test\cases\compiler.ts with a comment at the top of // @newline: LF , another one with CRLF, and one for wrong input as well.
  • add handling for the new switch in harnes.ts
    and in fileMetadataNames
  • you would need to pass then the correct newline value to getNewLine in harness.ts
  • jake runtests
  • jake baseline-accept
@kmashint

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2015

I found and started to make the harness.ts adjustments; the runtests is taking forever so I'll check in the morning.

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

commented Apr 28, 2015

the runtests is taking forever

Yeah, there's been some perf drop in recent versions of Node that will make it...worse. I've been on 0.10.33. I've recently been working to parallelize the tests.

@kmashint

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2015

Stalled this week catching up on paid work ... should have some time on the weekend to work on unit tests.

@kmashint

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2015

I thought I had this working locally, but I'll try to find other examples of an expected failure in unit test cases.

  1. compiler tests for tests/cases/compiler/newLineFlagWithCR.ts "before all" hook:
    Error: Unknown option for newLine: CR
@@ -822,7 +825,8 @@ module Harness {
scriptTarget: ts.ScriptTarget,
useCaseSensitiveFileNames: boolean,
// the currentDirectory is needed for rwcRunner to passed in specified current directory to compiler host
currentDirectory?: string): ts.CompilerHost {
currentDirectory?: string,
newLineKind?: ts.NewLineKind): ts.CompilerHost {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 2, 2015

Member

Don't use tabs

@@ -0,0 +1,4 @@
// @newline: CR

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 2, 2015

Member

We don't support CR so I guess we shouldn't have this test.

This comment has been minimized.

Copy link
@kmashint

kmashint May 2, 2015

Author Contributor

It was suggested to add a negative test for an invalid command-line arg but the test framework doesn't seem to support it so I'll remove the test.

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 2, 2015

Member

Sorry about any miscommunication - my understanding is that our harness doesn't support that sort of thing, though it's highly possible that @mhegazy might know something I'm not aware of.

newLine = setting.value;
if (setting.value.toLowerCase() === 'crlf') {
options.newLine = ts.NewLineKind.CarriageReturnLineFeed;
} else if (setting.value.toLowerCase() === 'lf') {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 2, 2015

Member

else on the next line (I know, the rest of the file is inconsistent)

options.newLine = ts.NewLineKind.CarriageReturnLineFeed;
} else if (setting.value.toLowerCase() === 'lf') {
options.newLine = ts.NewLineKind.LineFeed;
} else if (setting.value === '\\n') {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 2, 2015

Member

This doesn't look right- none of the other options work this way.

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 2, 2015

Member

Oh I see what happened; any idea how many other tests use that flag?

This comment has been minimized.

Copy link
@kmashint

kmashint May 2, 2015

Author Contributor

A grep search revealed only the contextualTyping.ts uses this other @newline flag as in the comment, no matches for @newlines plural.
// Handle old usage, e.g. contextualTyping.ts:// @newline: \n

I could use newlineflag for this new option, or change the old seldom-used option to something more internal, e.g. @normalizeNewline since it seems to be used in normalizeLineEndings(...).

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser May 2, 2015

Member

I don't even know why the heck we ever introduced that - I think @danquirk might've added it after we futzed with baselining issues, but I think it would be safe to assume we can use the LF flag here.

@kmashint

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2015

I've introduced a @ normalizenewline option to work as the old one did in contextualTyping.js. I'll adjust for merge conflicts tomorrow.

@kmashint kmashint force-pushed the kmashint:master branch from a6b7c6d to be3e3e7 May 3, 2015

@kmashint

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2015

OK, I've rebased my changes on top of microsoft/TypeScript. I tried using @ newline for both the new arguments and the old usage in contextualTyping.js but there were side-effects since the old usage affects not only the JS file but other debugging output files so I adjusted contextualTyping.js to use @ normalizenewline and all tests pass without further special treatment.

@mhegazy mhegazy merged commit be3e3e7 into microsoft:master May 4, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mhegazy

This comment has been minimized.

Copy link

commented May 4, 2015

Thanks @kmashint!

@kmashint

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2015

Happy to contribute in a small way, I hope others find it useful as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.