Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 315234 - [formatter] successive line comments are being indented an extra level
Summary: [formatter] successive line comments are being indented an extra level
Status: RESOLVED FIXED
Alias: None
Product: JSDT
Classification: WebTools
Component: General (show other bugs)
Version: 3.2   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.2 RC4   Edit
Assignee: Ian Tewksbury CLA
QA Contact: Nitin Dahyabhai CLA
URL:
Whiteboard: PMC_approved
Keywords:
: 249225 286654 312494 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-06-01 10:56 EDT by Ian Tewksbury CLA
Modified: 2011-10-20 10:45 EDT (History)
7 users (show)

See Also:
david_williams: pmc_approved+
thatnitind: pmc_approved? (raghunathan.srinivasan)
thatnitind: pmc_approved? (naci.dai)
deboer: pmc_approved+
neil.hauge: pmc_approved+
thatnitind: pmc_approved? (kaloyan)
cmjaun: review+
thatnitind: review+


Attachments
Fix Patch (965 bytes, patch)
2010-06-01 16:27 EDT, Ian Tewksbury CLA
no flags Details | Diff
Fix Patch - Update 1 (11.02 KB, patch)
2010-06-02 09:09 EDT, Ian Tewksbury CLA
thatnitind: iplog+
Details | Diff
functionality patch (965 bytes, patch)
2010-06-02 19:33 EDT, Nitin Dahyabhai CLA
no flags Details | Diff
tests patch (10.11 KB, patch)
2010-06-02 19:33 EDT, Nitin Dahyabhai CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Tewksbury CLA 2010-06-01 10:56:49 EDT
In the JS editor successive line comments are being indented an extra level when using the formatter

EXAMPLE:
dojo.declare("myDojo.Test", [],  {
// this is a really long comment that will have to be wrapped into multiple lines because it is so very very long
// this is a really long comment that will have to be wrapped into multiple lines because it is so very very long
// Constructor function. Called when instance of this class is created
constructor : function() {

}
});

RESULT:
dojo.declare("myDojo.Test", [], {
	// this is a really long comment that will have to be wrapped into multiple
	// lines because it is so very very long
		// this is a really long comment that will have to be wrapped into
		// multiple lines because it is so very very long
		// Constructor function. Called when instance of this class is created
		constructor : function() {

		}
	});

EXPECTED:
dojo.declare("myDojo.Test", [], {
	// this is a really long comment that will have to be wrapped into multiple
	// lines because it is so very very long
	// this is a really long comment that will have to be wrapped into multiple
	// lines because it is so very very long
	// Constructor function. Called when instance of this class is created
	constructor : function() {

	}
});
Comment 1 Ian Tewksbury CLA 2010-06-01 16:27:31 EDT
Created attachment 170697 [details]
Fix Patch

This is my proposed fix.  It fixes the issue and I do not see any regressions or other side effects.  It passes all of the existing formatting tests but the existing formatting tests do not seem to go through this code path.  I have been trying to make JUnits that due go through the code path in question but its truing out to be trickier then I hoped it would be.  Looks like the tests will have to actually create an viewer and run the formatting through the viewer due to the multiple formatters that get run through that code path.
Comment 2 Ian Tewksbury CLA 2010-06-02 09:09:31 EDT
Created attachment 170792 [details]
Fix Patch - Update 1

Same fix now with JUnits to test for regression.
Comment 3 Nitin Dahyabhai CLA 2010-06-02 19:20:25 EDT
* Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. 

This alters the source in a manner unexpected, and hard to detect, by the user.  It also affects AST rewrite, and thus any sources constructed purely through API calls.

* Is there a work-around? If so, why do you believe the work-around is insufficient? 

None.

* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? 

Manual testing plus JUnit.

* Give a brief technical overview. Who has reviewed this fix? 

Changes the formatter so that if two successive line comments are both over the maximum allowed length, breaking the first into two lines does not further indent the following lines.  Chris and I have reviewed the changes.

* What is the risk associated with this fix? 

Low as it is limited to formatting of line comments; block comments and other statements are unaffected.
Comment 4 David Williams CLA 2010-06-02 19:25:44 EDT
I wouldn't say "major" but source formatting is near and dear :) 

But, I'm not sure I understand the patch. On the surface it appears it just junits? If both, can you provide separate patches for junit vs. function? 

thanks,
Comment 5 Nitin Dahyabhai CLA 2010-06-02 19:33:18 EDT
Created attachment 170895 [details]
functionality patch
Comment 6 Nitin Dahyabhai CLA 2010-06-02 19:33:39 EDT
Created attachment 170896 [details]
tests patch
Comment 7 Nitin Dahyabhai CLA 2010-06-02 19:35:24 EDT
(In reply to comment #4)
> I wouldn't say "major" but source formatting is near and dear :) 
> 
> But, I'm not sure I understand the patch. On the surface it appears it just
> junits? If both, can you provide separate patches for junit vs. function? 
> 
> thanks,

Sure.
Comment 8 David Williams CLA 2010-06-02 19:47:15 EDT
k, thanks. Looks simple enough the payoff is worth the low risk.

I know in some cases mis-formaatting source is seen by users as "damaging their assests" so I think a good idea to fix now.
Comment 9 Neil Hauge CLA 2010-06-02 21:49:59 EDT
Approved based on isolated, low risk fix.
Comment 10 Nitin Dahyabhai CLA 2010-06-03 12:42:33 EDT
Released v201006030742.
Comment 11 Chris Jaun CLA 2010-06-21 13:57:29 EDT
*** Bug 312494 has been marked as a duplicate of this bug. ***
Comment 12 Chris Jaun CLA 2010-06-22 13:35:17 EDT
*** Bug 286654 has been marked as a duplicate of this bug. ***
Comment 13 Chris Jaun CLA 2010-06-22 14:18:28 EDT
*** Bug 249225 has been marked as a duplicate of this bug. ***
Comment 14 Ferran Basora CLA 2011-10-20 10:45:35 EDT
Could you test this code? In my Eclipse fails.

function func1 () {
				if (true) {
		return true;
	}
}

	function func2() {
}

But, with the semicolons at the end of functions, works for me:

function func1 () {
	if (true) {
		return true;
	}
};

function func2() {
};