Format block-comments better #3132

Merged
merged 3 commits into from Oct 20, 2013

5 participants

@caitp

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 caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
Caitlin Potter Fixes #3132 - Improve rendering of block-comments 54513f3
@caitp

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 caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
Caitlin Potter Fixes #3132 - Improve rendering of block-comments 1ca9ed5
@caitp caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
Caitlin Potter Fixes #3132 - Add tests for pretty block-comments 22bc8bf
@caitp

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 caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
Caitlin Potter Fixes #3132 - Add tests for pretty block-comments b4e2a13
@vendethiel
Collaborator

The compiler is written is coffee and is in src/

@caitp caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
Caitlin Potter Fixes #3132 - Improve rendering of block-comments a1a74f5
@caitp caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
Caitlin Potter Fixes #3132 - Improve rendering of block-comments 2877dd6
@caitp caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 23, 2013
Caitlin Potter Fixes #3132 - Improve rendering of block-comments 64e62c0
@caitp

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

@caitp caitp pushed a commit to caitp/coffee-script that referenced this pull request Aug 24, 2013
Caitlin Potter Fixes #3132 - Improve rendering of block-comments e3e51a0
@kof

+1

@caitp

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

👍 … 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

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 sjorek referenced this pull request in caitp/coffee-script Sep 17, 2013
Merged

Enhancement: Add more block-comment related tests #1

@sjorek

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

Caitlin Potter Merge pull request #1 from sjorek/issue-3132
Enhancement: Add more block-comment related tests
359e172
@jashkenas
Owner

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

@jashkenas jashkenas merged commit 302a46d into jashkenas:master Oct 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment