Bug 311578 - [formatter] Enable/disable tag detection should include comment start/end tokens
Summary: [formatter] Enable/disable tag detection should include comment start/end tokens
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.6   Edit
Hardware: All All
: P3 minor (vote)
Target Milestone: 3.7 M2   Edit
Assignee: Frederic Fusier CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-04 12:08 EDT by Markus Keller CLA
Modified: 2010-09-14 05:39 EDT (History)
4 users (show)

See Also:
markus.kell.r: review+


Attachments
Proposed patch (23.45 KB, patch)
2010-08-12 06:13 EDT, Frederic Fusier CLA
no flags Details | Diff
New proposed patch (32.92 KB, patch)
2010-08-19 06:12 EDT, Frederic Fusier CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2010-05-04 12:08:12 EDT
HEAD

The enable/disable tag detection from bug 27079 should include the comment start/end tokens.

If I want to use Jalopy-style "//J-" and "//J+" comments (see
http://jalopy.sourceforge.net/existing/comments.html ), I cannot set these strings in the preferences, since the formatter does not include the "//" when trying to find tags.

I would have to define the tags as "J-" and "J+", but that may produce unexpected matches in other comments (and it matches anywhere in a comment).


Example:

package a;
public class Bug {
int a      =  -     1  +    42;

//J-
int b      =  -     1  +    42;
//J+

char                       x;

////J-
int c      =  -     1  +    42;
////J+

char                       y;

/* J- */
int d      =  -     1  +    42;
/* J+ */

char                       z;

/* //J- */
int e      =  -     1  +    42;
/* //J+ */

/** J-1 blabla */
char                       t;
}
Comment 1 Deepak Azad CLA 2010-05-04 12:58:03 EDT
I like the tags in the initial bug report for this feature (bug 27079)

/* here goes my constants: @formatter:off */
public static final int FIRST            = 1;
public static final int SECOND           = 2;
public static final int AND_THE_LAST_ONE = 3;
/* @formatter:on */

- I have removed 1 '@' to make the tags '@formatter: off' and '@formatter: on'
- The tags can be used anywhere in a comment.
- They are easy to read (at least to me) and self explanatory.
- //J- and //J+ look a bit cryptic to me.
Comment 2 Olivier Thomann CLA 2010-05-04 14:56:49 EDT
For me, I don't like (J+, J-) at lot. J stands for Jalopy.
I much prefer: @formatter:off, @formatter:on.
Even if //J-, //J+ might be used by Jalopy users, I would not use the same tags.
Comment 3 Markus Keller CLA 2010-05-04 15:52:31 EDT
This bug is not about choosing defaults (use bug 311617 for that).

This bug is about *allowing* tags like "//J-".
Comment 4 Olivier Thomann CLA 2010-05-04 16:54:52 EDT
(In reply to comment #3)
> This bug is not about choosing defaults (use bug 311617 for that).
> This bug is about *allowing* tags like "//J-".
I know, but if //J- or //J+ is not in the picture anymore, then there is no need to fix this.
Comment 5 Markus Keller CLA 2010-05-04 17:15:29 EDT
> I know, but if //J- or //J+ is not in the picture anymore, then there is no
> need to fix this.

I agree that there's no immediate need to fix this if we don't use //J- as default. But it would still be nice to have good support for tags that are known to exist out in the field.
Comment 6 Olivier Thomann CLA 2010-05-05 13:50:32 EDT
Yes, we should target 3.7 for this.
Comment 7 Frederic Fusier CLA 2010-08-11 10:13:40 EDT
As the behavior of these tags would be really different than those implemented through bug 27079, I'd rather prefer to have a new formatter preference saying that jalopy tags should be used.

Something like FORMATTER_USE_JALOPY_ON_OFF_TAGS (not sure whether Jalopy name could be explicitly used though)...

Then, as described in Jalopy documentation, only line comments like:
//J-
//J+
would be significant for the formatter to switch off/on...

E.g. in the comment 0 sample, only the line of the local variable 'b' would not be formatted. All other comments will be ignored by the formatter...

Would that address your expectation Markus?
Comment 8 Dani Megert CLA 2010-08-11 11:30:24 EDT
-1 to add yet another preference for this.
Comment 9 Markus Keller CLA 2010-08-11 12:38:20 EDT
> E.g. in the comment 0 sample, only the line of the local variable 'b' would
> not be formatted. All other comments will be ignored by the formatter...
> 
> Would that address your expectation Markus?

Yes, but I agree with Dani that we shouldn't add yet another preference, especially not one that works only for a single product.

We should just do the right thing automatically when the given tag is a valid comment (line or block comment).

E.g.:
- tags "//J-" and "//J+" (without quotes) would not format 'b' in comment 0, but would format all other examples
- tags "/* J- */" and "/* J+ */" would not format 'd', but would format all other examples

I know this would change the behavior for 'e', but I would accept that. If you want to be super-conservative, you could also make "//J-" work for 'b' and 'e'.
Comment 10 Frederic Fusier CLA 2010-08-12 06:13:02 EDT
Created attachment 176448 [details]
Proposed patch

Patch addressing this issue without adding a new preference.

With this patch, the formatter will behave as follows:
- tags "//J-" and "//J+" (without quotes) would not format 'b', 'c' and 'e' (I used the super-conservative option ;-)) in comment 0 but would format all other examples
- tags "/* J- */" and "/* J+ */" would not format 'd' in comment 0, but would format all other examples
Comment 11 Markus Keller CLA 2010-08-16 14:40:17 EDT
I've tested the patch with some more examples, and I found it quite hard to understand why "//J-" and "//J+" don't stop the formatter in f and g below.

A sentence about the new behavior also wouldn't hurt in DefaultCodeFormatterConstants.FORMATTER_{DIS/EN}ABLING_TAG.


char                       z2;

//J-1
int f      =  -     1  +    42;
//J+2

char                       z3;

//J- 1
int g      =  -     1  +    42;
//J+ 2

char                       z4;

  //J-
int h      =  -     1  +    42;
  //J+

char                       z5;

/*
//J-
*/
int i      =  -     1  +    42;
/*
 //J+
 */

char                       z6;
Comment 12 Frederic Fusier CLA 2010-08-19 06:12:40 EDT
Created attachment 176976 [details]
New proposed patch

This patch addresses the issues described in comment 11
Comment 13 Markus Keller CLA 2010-08-30 15:32:32 EDT
Comment 12 looks good.
Comment 14 Frederic Fusier CLA 2010-08-31 03:04:23 EDT
(In reply to comment #12)
> Created an attachment (id=176976) [details]
> New proposed patch
> 
Released for 3.7M2 in HEAD stream.
Comment 15 Satyam Kandula CLA 2010-09-14 05:39:37 EDT
Verified for 3.7M2 using build I20100909-1700