FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Compiler flag to specify line ending by kmashint · Pull Request #2921 · microsoft/TypeScript · GitHub

Compiler flag to specify line ending - #2921

Merged
Mohamed Hegazy (mhegazy) merged 9 commits into
microsoft:masterfrom
kmashint:master
May 4, 2015
Merged

Compiler flag to specify line ending#2921
Mohamed Hegazy (mhegazy) merged 9 commits into
microsoft:masterfrom
kmashint:master

Conversation

Copy link
Copy Markdown
Contributor

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.

Hi Keith Mashinter (@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;

Daniel Rosenwasser (DanielRosenwasser) changed the title Compiler flag to specify line ending #1693 Compiler flag to specify line ending Apr 26, 2015
Comment thread src/compiler/diagnosticMessages.json Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

'--newLine' in single quotes.

Copy link
Copy Markdown
Contributor Author

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

Comment thread src/compiler/program.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

formatting, and use const.

Copy link
Copy Markdown
Contributor

Thanks Keith Mashinter (@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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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

Comment thread src/harness/harness.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Don't use tabs

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

Thanks Keith Mashinter (@kmashint)!

Copy link
Copy Markdown
Contributor Author

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL