Skip to content

remove an odd new-line for catch clause#13750

Merged
mhegazy merged 1 commit into
microsoft:masterfrom
gdh1995:fix-typo-in-catch
Jan 31, 2017
Merged

remove an odd new-line for catch clause#13750
mhegazy merged 1 commit into
microsoft:masterfrom
gdh1995:fix-typo-in-catch

Conversation

@gdh1995
Copy link
Copy Markdown
Contributor

@gdh1995 gdh1995 commented Jan 30, 2017

A new line has been written by writeLineOrSpace in emitTryStatement, so the first writeLine in emitCatchClause is not needed.

I think this is a bug because: if I modify emitTryStatement and make it write a space after try block, I'll get } + (space) + \n + indentation + catch ( + ... - The \n is obviously redundant.

@msftclas
Copy link
Copy Markdown

Hi @gdh1995, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

Comment thread src/compiler/emitter.ts
}

function emitCatchClause(node: CatchClause) {
writeLine();
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.

what about single line comments before the catch keyword:

try {

} //ddd
catch (x) { }

Copy link
Copy Markdown
Contributor Author

@gdh1995 gdh1995 Jan 31, 2017

Choose a reason for hiding this comment

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

Oops, I'll try to solve it.

Updated: The comment seems to have no influence.

@gdh1995
Copy link
Copy Markdown
Contributor Author

gdh1995 commented Jan 31, 2017

@mhegazy Here's my tests:

  1. multiline try block
try {
   var a = 1;
   var b = 2;
} // a
catch (e) {}

And this PR produces:

try {
    var a = 1;
    var b = 2;
} // a
catch (e) { }

And without comments:

try {
   let a = 1;
   let b = 2;
}
catch (e) {}
try {
    var a = 1;
    var b = 2;
}
catch (e) { }
  1. one-line block
try {} // a
catch (e) {}
try { } // a
catch (e) { }
try {}
catch (e) {}
try { }
catch (e) { }

@mhegazy mhegazy merged commit cf20850 into microsoft:master Jan 31, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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.

4 participants