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

Format block-comments better #3132

Merged
merged 3 commits into from
Oct 20, 2013
Merged

Format block-comments better #3132

merged 3 commits into from
Oct 20, 2013

Conversation

caitp
Copy link

@caitp caitp commented Aug 23, 2013

This is probably not a big deal, but I just don't like the way this works:

###
# This is a block comment, with important details!
###

Outputs to

/*
# This is a block comment, with important details!
*/

I feel like it would be better to format block comments more like

/*
 * This is a block comment, with important details!
 */

I don't think this would be terribly difficult to implement - so I might take a stab at it. The only reason it bothers me is because it looks so unclean and weird.

So, I dunno, is it worth taking a stab at?

caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
@caitp
Copy link
Author

caitp commented Aug 23, 2013

I'm not sure how to write this as an automated test (I need to verify the output generated by the compiler as a string...), but I have tested this manually:

###
# Block comment hngggg
###

blah = ->
  ###
  # Indented block comment
  ###
  undefined

###
# Mixed
with and without starting hash
###

barfunkle = ->
  ###
And again
  # Indented this time...
  ###
  undefined

Outputs:

// Generated by CoffeeScript 1.6.3

/*
 * Block comment hngggg
 */

(function() {
  var barfunkle, blah;

  blah = function() {

    /*
     * Indented block comment
     */
    return void 0;
  };


  /*
   * Mixed
  with and without starting hash
   */

  barfunkle = function() {

    /*
    And again
     * Indented this time...
     */
    return void 0;
  };

}).call(this);

I think this is pretty sweet, I'm just a bit worried about it potentially breaking some preprocessors or something. I don't really expect it to, but it wouldn't be unreasonable really.

Anyways, if this seems like something people might like, let me know how I can turn this into an automated test.

caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
@caitp
Copy link
Author

caitp commented Aug 23, 2013

Added a couple of basic tests, similar to the ones demonstrated in this issue (#3 is a multiline string, but can be shrunk down if needed)

caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
@vendethiel
Copy link
Collaborator

The compiler is written is coffee and is in src/

caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
@caitp
Copy link
Author

caitp commented Aug 23, 2013

@Nami-Doc thanks, I've added changes into src/nodes.coffee as well

caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 24, 2013
@kof
Copy link

kof commented Aug 31, 2013

+1

@caitp
Copy link
Author

caitp commented Sep 16, 2013

Hmm, I wonder if anyone would be willing to give this a look, in my opinion people who use coffeescript sources but distribute compiled JS would get a lot of value out of this, at least for non-minified releases. It kind of sucks to have really ugly code generated, and this should improve that a bit -- so I'd like to see this merged. Just let me know if there's anything I can do to improve the patch, thanks

@sjorek
Copy link
Contributor

sjorek commented Sep 17, 2013

👍 … tested, works, I like it = +1 !

The patch would be even more adorable if you adjust the final space only if it's not a single-line block-comment (move the final space-character from */ into the preceding ternary conditional … ? "\n" + this.tab + " " : …) This assumption is wrong, as this pull-request has shown: caitp#1

Additionally I'd like to sum up the syntax-definitions which are supported, because I miss some of them in the tests:

Input _(New)_:

### Single-line block-comment without additional space here => ###

Output (optimized by the suggestion above):

/* Single-line block-comment without additional space here => */

Input:

###
No leading hash-mark (#) with needless final space …
###

Output:

/*
No leading hash-mark (#) with needless final space …
 */

Input:

###
# Leading hash-mark (#) with the previously missing additional space. Perfect !
###

Output:

/*
 * Leading hash-mark (#) with the previously missing additional space. Perfect !
 */

Input _(New)_, useful for @doctags, see http://usejsdoc.org/about-getting-started.html:

###* (<= note the additional star)
# Initial star, leading hash-mark (#) plus the previously missing additional space. Perfect !
###

Output:

/** (<= note the additional star)
 * Initial star, leading hash-mark (#) plus the previously missing additional space. Perfect !
 */

Input _(New)_, same as above, but uses the raw * string directly in the comment:

###
 * Raw ` *` string with the previously missing additional space. Perfect !
###

Output:

/*
 * Raw ` *` string with the previously missing additional space. Perfect !
 */

Do you agree with my suggestion (implementing two or three more tests) ? Shall I prepare a pull-request for your branch ? done

Cheers,
Stephan

@caitp
Copy link
Author

caitp commented Sep 17, 2013

A patch to fix the 'unwanted extra space' (in cases where it is unwanted) is welcome, I can't look too closely at it for a while due to other projects, but I'll give it some review -- Yeah, more tests are always welcome, this is good stuff.

@sjorek
Copy link
Contributor

sjorek commented Sep 17, 2013

And here you are … 😀 the patch is not necessary as the above offered pull-request proves …

Enhancement: Add more block-comment related tests
@jashkenas
Copy link
Owner

I'm a bit wary of beefing up block comment output ... but hey, what the heck. Lovely work, @caitp.

jashkenas added a commit that referenced this pull request Oct 20, 2013
Format block-comments better
@jashkenas jashkenas merged commit 302a46d into jashkenas:master Oct 20, 2013
@GeoffreyBooth GeoffreyBooth mentioned this pull request Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants