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

Keep a single-line herecomment as a single-line js comment. #2929

Merged
merged 1 commit into from Apr 15, 2013

Conversation

Projects
None yet
4 participants
@lucasb-eyer
Contributor

lucasb-eyer commented Apr 15, 2013

Before:

###Foobar###
###
baz
###

becomes

/*Foobar
*/
/*
baz
*/

After:

###Foobar###
###
baz
###

becomes

/*Foobar*/
/*
baz
*/

The reason...

...for this is that some frameworks/tools use specially crafted comments as directives. In the former case, since they use /*{ and }*/ as delimiters, it is impossible to use coffeescript because of the current behavior. With this pull-request, it becomes possible by using ###{ and }### on one line.

Last and least,

/* Foo
*/

just looks wrong.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 15, 2013

Collaborator

IIRC, we were asked to add the newline for some other crazy semantic-comment reasoning. @Nami-Doc do you know which issue that was?

Collaborator

michaelficarra commented Apr 15, 2013

IIRC, we were asked to add the newline for some other crazy semantic-comment reasoning. @Nami-Doc do you know which issue that was?

@vendethiel

This comment has been minimized.

Show comment
Hide comment
@vendethiel

vendethiel Apr 15, 2013

Collaborator

#2889 ? (lol)

Collaborator

vendethiel commented Apr 15, 2013

#2889 ? (lol)

@lucasb-eyer

This comment has been minimized.

Show comment
Hide comment
@lucasb-eyer

lucasb-eyer Apr 15, 2013

Contributor

#2889 isn't the same though, as it talks about the newline after the closing */, while I talk about the newline before it.

Also, I do not request this because "it would look better" (I couldn't care less), but because it renders me unable to use coffee with Turbulenz! The "just looks wrong" part of my pull request comment was a halfhearted joke, hence the "last and least".

Edit: All I could find was:

  • #1186, which only talks about "readability" and multiline-herecomments.
  • #193, which was just a discussion that never ended.
    Note that my pull request only changes the output if the herecomment is on a single line. As soon as there is a \n in the herecomment, it nothing changes.
Contributor

lucasb-eyer commented Apr 15, 2013

#2889 isn't the same though, as it talks about the newline after the closing */, while I talk about the newline before it.

Also, I do not request this because "it would look better" (I couldn't care less), but because it renders me unable to use coffee with Turbulenz! The "just looks wrong" part of my pull request comment was a halfhearted joke, hence the "last and least".

Edit: All I could find was:

  • #1186, which only talks about "readability" and multiline-herecomments.
  • #193, which was just a discussion that never ended.
    Note that my pull request only changes the output if the herecomment is on a single line. As soon as there is a \n in the herecomment, it nothing changes.
@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 15, 2013

Collaborator

Alright, I'm convinced. Compile, amend, force push, and I'll merge.

Collaborator

michaelficarra commented Apr 15, 2013

Alright, I'm convinced. Compile, amend, force push, and I'll merge.

@lucasb-eyer

This comment has been minimized.

Show comment
Hide comment
@lucasb-eyer

lucasb-eyer Apr 15, 2013

Contributor

Done. Thanks!

Contributor

lucasb-eyer commented Apr 15, 2013

Done. Thanks!

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 15, 2013

Collaborator

Actually, now that I look at it more closely, this should really be rewritten using an interpolation. All new code should follow our ideal style.

code = "/*#{multident @comment, @tab}#{if '\n' in @comment then "\n#{@tab}" else ''}*/\n"
Collaborator

michaelficarra commented Apr 15, 2013

Actually, now that I look at it more closely, this should really be rewritten using an interpolation. All new code should follow our ideal style.

code = "/*#{multident @comment, @tab}#{if '\n' in @comment then "\n#{@tab}" else ''}*/\n"
@lucasb-eyer

This comment has been minimized.

Show comment
Hide comment
@lucasb-eyer

lucasb-eyer Apr 15, 2013

Contributor

Shall I amend & forcepush that again, or just add another commit?

Contributor

lucasb-eyer commented Apr 15, 2013

Shall I amend & forcepush that again, or just add another commit?

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 15, 2013

Collaborator

Same process, please.

Collaborator

michaelficarra commented Apr 15, 2013

Same process, please.

@satyr

This comment has been minimized.

Show comment
Hide comment
@satyr

satyr Apr 15, 2013

Collaborator

@michaelficarra: IIRC, we were asked to add the newline for some other crazy semantic-comment reasoning.

For the record, you added this extra/inner newline at 35a30fb in addition to that requested/outer newline.

Collaborator

satyr commented Apr 15, 2013

@michaelficarra: IIRC, we were asked to add the newline for some other crazy semantic-comment reasoning.

For the record, you added this extra/inner newline at 35a30fb in addition to that requested/outer newline.

@vendethiel

This comment has been minimized.

Show comment
Hide comment
@vendethiel

vendethiel Apr 15, 2013

Collaborator

Now that's next level here, thanks @satyr -- I don't believe I was even here back then

Collaborator

vendethiel commented Apr 15, 2013

Now that's next level here, thanks @satyr -- I don't believe I was even here back then

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 15, 2013

Collaborator

@satyr: perfect 😄. Then we can change it with certainty.

Collaborator

michaelficarra commented Apr 15, 2013

@satyr: perfect 😄. Then we can change it with certainty.

@lucasb-eyer

This comment has been minimized.

Show comment
Hide comment
@lucasb-eyer

lucasb-eyer Apr 15, 2013

Contributor

nice, thanks. (Requested changes done.)

Contributor

lucasb-eyer commented Apr 15, 2013

nice, thanks. (Requested changes done.)

@vendethiel

This comment has been minimized.

Show comment
Hide comment
@vendethiel

vendethiel Apr 15, 2013

Collaborator

Does that apply?

Actually, now that I look at it more closely, this should really be rewritten using an interpolation. All new code should follow our ideal style.

Collaborator

vendethiel commented Apr 15, 2013

Does that apply?

Actually, now that I look at it more closely, this should really be rewritten using an interpolation. All new code should follow our ideal style.

@lucasb-eyer

This comment has been minimized.

Show comment
Hide comment
@lucasb-eyer

lucasb-eyer Apr 15, 2013

Contributor

Now it should; made a mistake before your comment.

Contributor

lucasb-eyer commented Apr 15, 2013

Now it should; made a mistake before your comment.

@vendethiel

This comment has been minimized.

Show comment
Hide comment
@vendethiel

vendethiel Apr 15, 2013

Collaborator

great - sorry for these :p.

Collaborator

vendethiel commented Apr 15, 2013

great - sorry for these :p.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Apr 15, 2013

Collaborator

LGTM. Merging.

Collaborator

michaelficarra commented Apr 15, 2013

LGTM. Merging.

michaelficarra added a commit that referenced this pull request Apr 15, 2013

Merge pull request #2929 from lucasb-eyer/master
Keep a single-line herecomment as a single-line js comment.

@michaelficarra michaelficarra merged commit c785e00 into jashkenas:master Apr 15, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment