Skip to content

Shebang #4120

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

Merged
merged 5 commits into from
Aug 4, 2015
Merged

Shebang #4120

merged 5 commits into from
Aug 4, 2015

Conversation

basarat
Copy link
Contributor

@basarat basarat commented Aug 2, 2015

Closes #2749

Appreciate a review by @CyrusNajmabadi : Emit ended up as a strategic call similar to detached comments

Also wrote : http://basarat.gitbooks.io/typescript/content/docs/compiler/contributing.html since I was in the area 🌹



//// [shebang.js]
#!/usr/bin/env node
Copy link
Contributor

Choose a reason for hiding this comment

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

So, does node ignore shebangs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add tests where the # isn't on the first position. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 🌹

@CyrusNajmabadi
Copy link
Contributor

Looks pretty good to me. With a couple of changes this should be good to go in. I'd like @ahejlsberg to weigh in if we want to go ahead with them.

My only concern is how this affects our ability to do something like add support for #pp-directives in the future. Or maybe that wouldn't be a problem as we'd still just detect #! as a special directive.

@basarat
Copy link
Contributor Author

basarat commented Aug 3, 2015

Done:

  • add tests where the #! isn't on the first position
  • CR based cleanup

/** Optionally, get the shebang */
export function getShebang(text: string): string {
return shebangTriviaRegex.test(text)
? shebangTriviaRegex.exec(text)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

too much indent.

@CyrusNajmabadi
Copy link
Contributor

@mhegazy You ok with this?

@@ -528,6 +538,20 @@ namespace ts {
return pos;
}

let shebangTriviaRegex = /^#!.*/;
Copy link
Contributor

Choose a reason for hiding this comment

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

const?

@basarat
Copy link
Contributor Author

basarat commented Aug 3, 2015

Done (as an amendment on the CR commit):

  • const for regex
  • further cleanup on code indentation

@mhegazy
Copy link
Contributor

mhegazy commented Aug 4, 2015

thanks!

mhegazy added a commit that referenced this pull request Aug 4, 2015
@mhegazy mhegazy merged commit d855f47 into microsoft:master Aug 4, 2015
@basarat basarat deleted the feat/shebang branch August 4, 2015 21:43
@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