Community
Participate
Working Groups
Build 3.0M8 I'm using the Code formatter with the Java Conventions profile (with only the maximum line width changed to 40 characters) to format this class: public class Main { public static void main( String[] args) { foo(bar(1, 2, 3), baz(4, 5, 6, 7)); } } The result looks like this: public class Main { public static void main( String[] args) { foo(bar(1, 2, 3), baz(4, 5, 6, 7)); } } I was expecting something like this: public class Main { public static void main( String[] args) { foo(bar(1, 2, 3), baz(4, 5, 6, 7)); } } The formatter should break the line in between the calls to bar and baz, so that each line can be read by itself (almost). In general, this means that lines should be broken at the highest level possible. Btw, I've noticed that lines with arithmetic expressions are already split this way, so subexpressions contained in paranthesis are usually kept on the same line. Witth the same settings, the following code: public class Main { public static void main( String[] args) { int a = (1 + 2 + 3) + (4 + 5 + 6 + 7); } } gets formatted like this: public class Main { public static void main( String[] args) { int a = (1 + 2 + 3) + (4 + 5 + 6 + 7); } }
Reconsider post 3.0.
Reconsidering for 3.1
I'll investigate it for M3.
I would generalize this bug as follows: "Line wrapping needs better aesthetics" Here's another example, this time mixing conditional operator and method calls. Eclipse 3.1 code formatter: String foo = thing.startsWith("/stuff/") ? thing.substring( "/stuff".length()) : thing; More aesthetic formatting: String foo = thing.startsWith("/stuff/") ? thing.substring("/stuff".length()) : thing; or (depending on your wrapping prefs) String foo = thing.startsWith("/stuff/") ? thing.substring("/stuff".length()) : thing; A variation using string concatenation instead of a conditional operator: String foo = thing.startsWith("/stuff/") ? "/" + "/stuff".length() + thing : thing; Would be more readable as: String foo = thing.startsWith("/stuff/") ? "/" + "/stuff".length() + thing : thing; I think the generalization is that "arguments" to the outermost "method call" shouldn't be wrapped unless necessary.
*** Bug 95735 has been marked as a duplicate of this bug. ***
Please add the option to disable this "Line wrapping better aesthetics" feature, because I'd like to format my code exactly like the formatter formats it now.
Moving to 3.3. This definitely needs to be addressed in the 3.3 time frame.
*** Bug 152444 has been marked as a duplicate of this bug. ***
*** Bug 164093 has been marked as a duplicate of this bug. ***
*** Bug 146175 has been marked as a duplicate of this bug. ***
*** Bug 130471 has been marked as a duplicate of this bug. ***
*** Bug 97613 has been marked as a duplicate of this bug. ***
Postponed again.
*** Bug 83915 has been marked as a duplicate of this bug. ***
*** Bug 99026 has been marked as a duplicate of this bug. ***
*** Bug 104004 has been marked as a duplicate of this bug. ***
*** Bug 104774 has been marked as a duplicate of this bug. ***
*** Bug 114639 has been marked as a duplicate of this bug. ***
*** Bug 117896 has been marked as a duplicate of this bug. ***
*** Bug 179796 has been marked as a duplicate of this bug. ***
We ran out of time again. Deferring post 3.4
I don't know about the current state, but from all the closed bugs, having the following formatting since years is a big problem: --start-- out .printf( "test %d test %d test %d test %d test %d test %d test %d test %d", 1, 2, 3, 4, 5, 6, 7); --end---
(In reply to comment #22) Exactly my point. Logical splitting by groups of code is nice and all, but for god's sake, splitting the code in ways that makes lines even longer than they were before strikes me as exceptionally stupid. Aesthetics can be a matter of taste, but splitting code so that the resulting line is even longer than the original line of code, seems a genuine bug to me... (+1 vote)
*** Bug 203588 has been marked as a duplicate of this bug. ***
I would love to see the Eclipse formatter able to do a formating like this: return new ToStringBuilder( this ) .append( "name", getName() ) .append( "category", getCategory() ) .append( "download", getDownloadUrl() ) .append( "detail", getDetailUrl() ) .append( "publisher", getPublisher() ) .append( "time", getPublishTime() ) .append( "users", getUsers() ) .toString(); (see http://dev.eclipse.org/newslists/news.eclipse.newcomer/msg10260.html) In our code we make often use of the Builder pattern, which allows for a very elegant way to do a lot of things. The only small downside till date has been that the Eclipse Java formatting of these constructs is horrible. Simply horrible. We now have to hunt down all occurrences of this pattern after an automatic format, correct the formatting manually, and then hope that someone else doesn't apply automatic formatting again later. How hard would adding such a formatting option be for an outsider from the community? Could someone maybe comment about that and maybe point us in the right direction?
I ran out of time for M6 for this bug as I needed to focus on bugs which required an API addition, hence move its target to M7...
(In reply to comment #8) > *** Bug 152444 has been marked as a duplicate of this bug. *** Bug 15244 was in fact a duplicate of bug 198074... (In reply to comment #11) > *** Bug 130471 has been marked as a duplicate of this bug. *** Bug 130471 was in fact a duplicate of bug 264112... (In reply to comment #14) > *** Bug 83915 has been marked as a duplicate of this bug. *** Bug 83915 was in fact a duplicate of bug 264112... (In reply to comment #15) > *** Bug 99026 has been marked as a duplicate of this bug. *** Bug 99026 was in fact a duplicate of bug 264112... (In reply to comment #16) > *** Bug 104004 has been marked as a duplicate of this bug. *** Bug 104004 was in fact a duplicate of bug 264112... (In reply to comment #17) > *** Bug 104774 has been marked as a duplicate of this bug. *** Bug 104774 was in fact a duplicate of bug 264112... (In reply to comment #18) > *** Bug 114639 has been marked as a duplicate of this bug. *** Bug 114639 was in fact a duplicate of bug 264112... (In reply to comment #19) > *** Bug 117896 has been marked as a duplicate of this bug. *** Bug 117896 was in fact a duplicate of bug 264112...
(In reply to comment #23) > (In reply to comment #22) > Exactly my point. > This was fixed in 3.6M5, see bug 264112
(In reply to comment #25) > I would love to see the Eclipse formatter able to do a formating like this: > > return new ToStringBuilder( this ) > .append( "name", getName() ) > .append( "category", getCategory() ) > .append( "download", getDownloadUrl() ) > .append( "detail", getDetailUrl() ) > .append( "publisher", getPublisher() ) > .append( "time", getPublishTime() ) > .append( "users", getUsers() ) > .toString(); > > (see http://dev.eclipse.org/newslists/news.eclipse.newcomer/msg10260.html) > > In our code we make often use of the Builder pattern, which allows for a very > elegant way to do a lot of things. The only small downside till date has been > that the Eclipse Java formatting of these constructs is horrible. Simply > horrible. We now have to hunt down all occurrences of this pattern after an > automatic format, correct the formatting manually, and then hope that someone > else doesn't apply automatic formatting again later. > > How hard would adding such a formatting option be for an outsider from the > community? Could someone maybe comment about that and maybe point us in the > right direction? This is a requirement to add a new formatter preference which would break line at each cascaded message sent. Please open a new bug for it. Waiting for next version about this requirement, 3.6 will allow you to format manually this kind of call and disable the formatter around it (see bug 27079)
(In reply to comment #4) > I would generalize this bug as follows: > > "Line wrapping needs better aesthetics" > > Here's another example, this time mixing conditional operator and method calls. > > Eclipse 3.1 code formatter: > > String foo = thing.startsWith("/stuff/") ? thing.substring( > "/stuff".length()) : thing; > > > More aesthetic formatting: > > String foo = thing.startsWith("/stuff/") > ? thing.substring("/stuff".length()) : thing; > > or (depending on your wrapping prefs) > > String foo = thing.startsWith("/stuff/") > ? thing.substring("/stuff".length()) > : thing; > > > A variation using string concatenation instead of a conditional operator: > > String foo = thing.startsWith("/stuff/") ? "/" + > "/stuff".length() + thing : thing; > > Would be more readable as: > > String foo = thing.startsWith("/stuff/") > ? "/" + "/stuff".length() + thing > : thing; > > > I think the generalization is that "arguments" to the outermost "method call" > shouldn't be wrapped unless necessary. This is a different requirement than the original opened issue. Please open a separate enhancement if you want this new behavior to be added to the formatter preferences...
Created attachment 165868 [details] Proposed patch Not perfect as there are still some corner cases which are difficult to address with the current design. But I think this patch improves a lot the formatting of the method calls...
Created attachment 165869 [details] Fix failing JDT-UI tests as the formatting output changed Markus, I want to release this patch for tomorrow (ooops today ;-)) warm-up build. We need to synchronize to be sure that those two patches are released in the same time frame... TIA
(In reply to comment #32) Looks good, Dani will release this. (In reply to comment #29) > This is a requirement to add a new formatter preference which would break line > at each cascaded message sent. Please open a new bug for it. > > Waiting for next version about this requirement, 3.6 will allow you to format > manually this kind of call and disable the formatter around it (see bug 27079) ... or you can select "Line Wrapping > Never join already wrapped lines" globally. Then, you can break it manually and the formatter will only format inside lines (or add additional line breaks if necessary).
(In reply to comment #31) > Created an attachment (id=165868) [details] > Proposed patch Released for 3.6M7 in HEAD stream.
(In reply to comment #5) > *** Bug 95735 has been marked as a duplicate of this bug. *** (In reply to comment #12) > *** Bug 97613 has been marked as a duplicate of this bug. *** (In reply to comment #9) > *** Bug 164093 has been marked as a duplicate of this bug. *** Those bugs will be finally considered as separated work and are no longer duplicates...
>Created an attachment (id=165869) [details] [diff] >Fix failing JDT-UI tests as the formatting output changed Committed to HEAD and released into I20100423-0800.
I dont know if this a corner case and has already been reported in a bug - but i found an inconsistency during verification. In the below example, try CTRL+SHIFT+F: class ABC { public String getString(String sadla, String ad, String qwer3q, String newPaasdram){ return "hi"; } public String foo(String a, String b) { return "hello"; } public static void main(String[] args) { ABC abc = new ABC(); abc.foo(abc.getString("sadldwdqd", "adadw", "qwdwwfr", "WFs"), abc.getString("sadlaawddwdqd", "adawqrdw", "qwdwqrfqwfr","dwqFRFRQEWFs")); } } The output obtained is: ... public static void main(String[] args) { ABC abc = new ABC(); abc.foo(abc.getString("sadldwdqd", "adadw", "qwdwwfr", "WFs"), abc .getString("sadlaawddwdqd", "adawqrdw", "qwdwqrfqwfr", "dwqFRFRQEWFs")); ... while this should be expected: ... public static void main(String[] args) { ABC abc = new ABC(); abc.foo(abc.getString("sadldwdqd", "adadw", "qwdwwfr", "WFs"), abc.getString("sadlaawddwdqd", "adawqrdw", "qwdwqrfqwfr", "dwqFRFRQEWFs")); ... Different lengths of strings passed as arguments give different outputs here. This works fine if the strings passed as arguments are small. Frederic, can you please verify this? I'll raise a separate defect if this isnt a known issue already.Thanks.
(In reply to comment #37) Ayush, please use an attached file or reduce the line length to make your test case readable, thanks
Created attachment 166198 [details] testcase
Oh and forgot to mention... I'm using java conventions default profile
(In reply to comment #37) > > Different lengths of strings passed as arguments give different outputs here. > This works fine if the strings passed as arguments are small. > Frederic, can you please verify this? I'll raise a separate defect if this isnt > a known issue already.Thanks. Yes, this is a corner case, where the second argument is over the max line, hence the formatter enters into the "panic" mode and breaks the line as it did before (i.e. the sooner as it can). Please open a new bug with this test case attached. It will be the starting point for the second round to definitely fix this issue...
(In reply to comment #41) > Yes, this is a corner case, where the second argument is over the max line, > hence the formatter enters into the "panic" mode and breaks the line as it did > before (i.e. the sooner as it can). > > Please open a new bug with this test case attached. It will be the starting > point for the second round to definitely fix this issue... Thanks. Opened bug 310642 as follow up.
Verified for 3.6M7 using build I20100426-0852.
Verified.
I believe this is related to this bug, let me know if I have to raise a new one instead. I have a line of code that resembles this : AcceleoUIActivator.getDefault().getLog().log(new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, e.getMessage(), e)); With our formatter, it was indented like this in 3.4, 3.5 and 3.6 (until M7 went live) : AcceleoUIActivator.getDefault().getLog().log( new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, e.getMessage(), e)); The problem is, it is now formatted as : AcceleoUIActivator .getDefault() .getLog() .log(new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, e.getMessage(), e)); With the _very same formatter_ in Eclipse 3.6RC1. This is not acceptable as our project is compatible with Eclipse 3.4, Eclipse 3.5 _and_ Eclipse 3.6. We have developpers on all three versions, and the code must look the same on all three (this is the very reason why we had a custom formatter defined on a per-project basis in the first place). This new formatting setting must be set to the "old" behavior by default, and allow users to use the "new" if they so desire ; it mustn't be forced on users. I for one don't like this new formatting, but in general I tend to consider it a bug when a configuration for something (here, the formatter) behaves differently on the same input with different Eclipses (save for bug fixes). Rambling apart, can this new setting be set to false by default or, if the behavioral change is not a regression and was desired, do I have a way to configure my formatter to use the old behavior without having my configuration fail in older Eclipses?
(In reply to comment #45) > I believe this is related to this bug, let me know if I have to raise a new one > instead. > The reason why we have not introduced a new preferences is that the previous behavior was obviously buggy (see all the complains along this bug and its duplicate...). Hence, we could not add a preference saying: 'keep the buggy behavior'. Obviously, I agree that in your case, the fixed behavior is not optimal... :-S So, please open a new bug for this peculiar issue and we'll try to address it as soon as possible. Waiting for the fix, which unfortunately would only be in 3.6.1, you can set those lines formatted as 3.5 did and disable the formatter around them using the disabling/enabling tags. E.g., if you use default tags that would look like: // @format:off AcceleoUIActivator.getDefault().getLog().log( new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, e.getMessage(), e)); // @format:on TIA
(In reply to comment #46) > (In reply to comment #45) > > I believe this is related to this bug, let me know if I have to raise a new one > > instead. > > > The reason why we have not introduced a new preferences is that the previous > behavior was obviously buggy (see all the complains along this bug and its > duplicate...). Hence, we could not add a preference saying: 'keep the buggy > behavior'. When I look at all of the changes I have when comparing my code (formatted in RC1) with the code a colleague commited (formatted with M6), I can't help but think it is the new behavior that is buggy ... There's no accounting for taste as they say ; the main problem being that we won't be able to carry on working with different eclipses without cluttering our code with "format : off" comments. And I say clutter : 120 files wrongly formatted, with up to 18 lines that are now formatted differently according to the Eclipse we develop with, that's at the _very_ least a good thousand useless comments in our code ... and that's a small code base. > > Obviously, I agree that in your case, the fixed behavior is not optimal... :-S > So, please open a new bug for this peculiar issue and we'll try to address it > as soon as possible. > I could give more examples of places where the code was better formatted in 3.5. The real problem here is compatibility. We may be the only ones with developpers on more than a single version of Eclipse (if only for our product's backward compatibility's sake), but I doubt it. With something as important as the JDT's formatter, I find it truly hallucinating to just say "we modified it, if you wish for a code formatted in a particular way, just format yourself and put "@format : off" comments". That really accounts to this : it may be seen as "buggy" or "ugly" by others, but I might want to always put new lines before and after my braces ; I might want to have no indentation in my files ... the formatter allows it. Why doesn't it allow for me to cut my lines wherever possible (dot, opening parenthesis, comma) instead of dot only? What was once formatted as AcceleoCommonPlugin.log(AcceleoCommonMessages.getString(INSTANTIATION_FAILURE_KEY, ((Class<?>)service).getName()), e, false); is now formatted as AcceleoCommonPlugin.log( AcceleoCommonMessages.getString(INSTANTIATION_FAILURE_KEY, ((Class<?>)service).getName()), e, false); What was once EPackage.Registry.INSTANCE.put(org.eclipse.ocl.expressions.ExpressionsPackage.eINSTANCE .getNsURI(), org.eclipse.ocl.expressions.ExpressionsPackage.eINSTANCE); is now EPackage.Registry.INSTANCE.put( org.eclipse.ocl.expressions.ExpressionsPackage.eINSTANCE.getNsURI(), org.eclipse.ocl.expressions.ExpressionsPackage.eINSTANCE); Well you get the idea. I am all for fixing bugs, and of course allowing users to keep a bug through a preference would be stupid. Here, we're not talking about a bug, we're talking about code preferences. I like the previous behavior of breaking lines wherever possible when the max width was reached, why can't I keep it? Even it it is set to the new policy by default, there should be a way to configure it. I thought RC1 was still early enough to fix bugs, especially one that has been introduced in M7 as we're very few to have tested it. I believe the code formatter is too much of a core component (I don't really know anyone using Eclipse but not using the formatter when targetting Java) to not consider a bug something that will prevent teams (at least mine) from using the 3.6.0 release. I do not think that cluttering our code with useless comments is an acceptable workaround, and I don't think manually checking all of the 120+ changes before each commit is either. I might be the only one on this huge cc list to think like that, but I think this will get more than a few reports after Helios goes live and folks begin using it.
Frédéric, how hard is it to add a preference? Obviously we wouldn't call it 'Keep broken behavior' but would have to come up with a good name that reflects the formatting preference chosen by Laurent - is that possible? If we add the preference we can still have the new (fixed) style as default but at least give the opportunity to back out of it by adjusting the formatter profile.
All formatting examples given in comment 47 look better to me now. All examples I have seen so far are improved. If we cannot fix issues with the formatter without adding new preferences to keep the old ugly formatting, because this might affect previous formatting, then we better spend our time working on something else. How do you want to call the new preference? We already have so many preferences... Adding one to keep the old behavior is not the right approach. This means that each time we fix something we need a new preference to keep the old bogus behavior. Is this really what you are asking ?
(In reply to comment #49) > All formatting examples given in comment 47 look better to me now. All examples > I have seen so far are improved. I mentionned it in comment 47, you confirm it in your own. This is no bug, this is a discussion about tastes. I for one prefer the old behavior for most of these samples, even if they are samples I randomly copy/pasted from my team synchronizing view. What of the example from comment 46? That one was among the ugliest I had. > If we cannot fix issues with the formatter without adding new preferences to > keep the old ugly formatting, because this might affect previous formatting, > then we better spend our time working on something else. > How do you want to call the new preference? We already have so many > preferences... Adding one to keep the old behavior is not the right approach. > This means that each time we fix something we need a new preference to keep the > old bogus behavior. Is this really what you are asking ? I'll make abstraction of the "bogus", I believe I already discussed that. For the rest, yes. If we are to break the formatting for the community at large, I'd rather a preference be introduced to let those that don't like the change, or teams with legacy code to maintain, keep the formatting they like and/or are accustomed to. Your alternative isn't really better : force all users to switch to 3.6M7 or later, format all of their projects, commit it and never try to go back to ol' Galileo ever again lest they have to manually double-check their code before any commit. That with the issues it can bring to projects that strive for compatibility with multiple Eclipse versions. Might not be for you, but we have users, clients, fellow developpers that are still using old Eclipses and cannot upgrade. Striving to maintain compatibility through API breakages of the frameworks we depend on is already hard enough, having to double-check each and every commit to review what really are changes and what are simply noise is an unwanted overhead. That being said, this is the very same formatter we have used since Eclipse 3.3 ; and 3.6M7 is the very first time it gets broken like that. I believe this doesn't mean that no bug has ever been fixed for the past 3 years. On the contrary, many preferences have been added to fine-tune the formatting of our code, and it's been a pleasure so far to work with something as stable and robust as that. I don't think adding another preference to let users choose how their lines should be broken (as I understand it, either "on nested call" or "whenever possible") would be that much of a deviation to the past years' evolution.
(In reply to comment #50) > > I mentionned it in comment 47, you confirm it in your own. This is no bug, > this is a discussion about tastes. I for one prefer the old behavior for most > of these samples, even if they are samples I randomly copy/pasted from my team > synchronizing view. What of the example from comment 46? That one was among > the ugliest I had. > I guess you meant comment 45. For this one I already agree that the current behavior deserve a new bug to be opened... > ... > That being said, this is the very same formatter we have used since Eclipse 3.3 > ; and 3.6M7 is the very first time it gets broken like that. I believe this > doesn't mean that no bug has ever been fixed for the past 3 years. On the > contrary, many preferences have been added to fine-tune the formatting of our > code, and it's been a pleasure so far to work with something as stable and > robust as that. I don't think adding another preference to let users choose how > their lines should be broken (as I understand it, either "on nested call" or > "whenever possible") would be that much of a deviation to the past years' > evolution. That's wrong, we strongly changed the formatter between 3.3 and 3.4 around comments formatting. And of course we didn't get it the first time, many bugs were opened at that time about this change and we worked hard to fix them all and finally succeeded to get an agreement on the new version when initial misbehaviors were fixed. But at that time, nobody complains about this compatibility. So, I guess that using three different versions of Eclipse to edit the same file might be a little bit a not so frequent configuration. BTW, have you also tried to use the 'Never join lines' preference? It seems that what you're asking for looks like this already existing preference...
Sorry, this'll be another lengthy post, got a little carried away with my examples. (In reply to comment #51) > (In reply to comment #50) > > > > I mentionned it in comment 47, you confirm it in your own. This is no bug, > > this is a discussion about tastes. I for one prefer the old behavior for most > > of these samples, even if they are samples I randomly copy/pasted from my team > > synchronizing view. What of the example from comment 46? That one was among > > the ugliest I had. > > > I guess you meant comment 45. For this one I already agree that the current > behavior deserve a new bug to be opened... Sorry, that should have been 45 indeed. > That's wrong, we strongly changed the formatter between 3.3 and 3.4 around > comments formatting. And of course we didn't get it the first time, many bugs > were opened at that time about this change and we worked hard to fix them all > and finally succeeded to get an agreement on the new version when initial > misbehaviors were fixed. > I guess we didn't have anything in our code base presenting this issue, as I don't recall encountering issues with comment formating. > > BTW, have you also tried to use the 'Never join lines' preference? It seems > that what you're asking for looks like this already existing preference... I have tried most if not all wrapping related options in order to try and have a coherent behavior, to no avail. The "never join already wrapped lines" preference was promising, but it really doesn't do the trick : new code scarcely gets properly formatted with this on. I agree that in most cases I don't mind the new behavior as it simply seems to put the source of method calls on the same line as the calls themselves. However it implies stranges things and I cannot single out the reason why some lines get formatted as they are. If I have to point out some of my strangest -----8<----- source.log(AcceleoParserMessages.getString( "AcceleoParser.Error.InvalidAST", oURI.lastSegment()), 0, -1); //$NON-NLS-1$ ----->8----- that is now -----8<----- source.log( AcceleoParserMessages.getString( "AcceleoParser.Error.InvalidAST", oURI.lastSegment()), 0, -1); //$NON-NLS-1$ ----->8----- -----8<----- AcceleoUIActivator.getDefault().getLog().log( new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages .getString("AcceleoLaunchDelegate.MissingProfileModel"))); //$NON-NLS-1$ ----->8----- which becomes -----8<----- AcceleoUIActivator .getDefault() .getLog() .log(new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages .getString("AcceleoLaunchDelegate.MissingProfileModel"))); //$NON-NLS-1$ ----->8----- Maybe due to the NON-NLS annotation? The worst of all seems to be -----8<----- AcceleoUIActivator.getDefault().getLog().log( new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages.getString( "AcceleoLaunchDelegate.ProfileModelError", new Object[] {e.getMessage(), }))); //$NON-NLS-1$ ----->8----- Which seems much the same to me, but becomes -----8<----- AcceleoUIActivator .getDefault() .getLog() .log(new Status( IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages .getString( "AcceleoLaunchDelegate.ProfileModelError", new Object[] {e.getMessage(), }))); //$NON-NLS-1$ ----->8----- ugh. -----8<----- IPath relativePath = fileEMTL.removeFileExtension().addFileExtension( IAcceleoConstants.MTL_FILE_EXTENSION).removeFirstSegments( folder.getFullPath().segmentCount()); ----->8----- now becomes -----8<----- IPath relativePath = fileEMTL.removeFileExtension() .addFileExtension(IAcceleoConstants.MTL_FILE_EXTENSION) .removeFirstSegments(folder.getFullPath().segmentCount()); ----->8----- Which seems to violate my thought that the formatter was now putting the source of an invocation before the call ... So many examples in the small code base of Acceleo, and that many doesn't allow me to understand the new rules at all. At some other points, the formatter seems to be using the rule to "break line on each argument" and things like -----8<----- proposals.add(createTemplateProposal(name, offset - start.length(), start.length(), name.length(), AcceleoUIActivator.getDefault().getImage( "icons/template-editor/completion/Type.gif"), eClassifier //$NON-NLS-1$ .getName(), null, name)); ----->8----- become -----8<----- proposals.add(createTemplateProposal( name, offset - start.length(), start.length(), name.length(), AcceleoUIActivator.getDefault().getImage( "icons/template-editor/completion/Type.gif"), eClassifier //$NON-NLS-1$ .getName(), null, name)); ----->8----- Note : the NON-NLS annotation isn't what makes the formatter go doofus here, I tried it with and without, and my line still gets broken on each argument. I'll stop with my samples here, you can get more of the sort by browsing the CVS on /cvsroot/modeling/org.eclipse.m2t/org.eclipse.acceleo/plugins and retrieving all of the projects contained in this folder. I believe all projects will have things like this showing up as soon as they switch to 3.6. I do agree that some of these can be seen as improvements ... but absolutely not all of them are ; if I were to vote on this, I'd obviously ask for the old behavior back, and I don't believe I'd be the only one. True, I may be the only one here that asks for this, and the 44 comments before (and on the duplicates) me seem to tell that this change is something wanted by part of the community ... but we scarcely see satisfied customers on a bugzilla (proof being me, happy for a number of releases and realising there was a bug like this only when struck). Surely I am not the only one that will be dissatisfied by such a change; especially with the bugs it introduces (those one or two of my samples that get totally distorted) and no way to workaround them (save for disabling the formatter and doing it themselves, talk about regressing!), but I might very well be the only one of those that gave a try to M7/RC1.
I predicted this problem 4 years ago - please see my comment 6.
My 2 cents. Given the fact that anyone can open new bug reports about the new formatter strategy to highlight cases where the formatter behaves worst than before, I can't understand why a new formatter would cause so many problems to existing code. I mean: is anyone forcing your team (Laurent) to reformat the whole class when editing an existing source file? I personally work in a team where not only there are users using different versions of Eclipse, but some of them are even using NetBeans. That said, in my experience it's always a bad idea to make the IDE reformat whole classes, so I set up a save action to just reformat the edited lines. When this is not appropriate (for whatever reason), I manually reformat single blocks of code through selection. On the other hand, giving vacant reads to this bug report there's only one thing that worries me about the new formatter strategy (but I may be perfectly wrong): my feeling is that while the old formatter had a quite predictable behaviour, the new one seems to handle many particular cases in different ways, so that it might be hard to predict the result. I mean, at a first thought it's quite unexpected the fact that, given the exact same style preferences, the formatter behaves so much differently than before in so many cases. Maybe the optimal solution would be to translate the new behaviours in appropriate new formatter preferences, as Laurent said?
(In reply to comment #53) > I predicted this problem 4 years ago - please see my comment 6. Thanks, at least I know that we're not the only ones that don't like the new aesthetics. (In reply to comment #54) > [...] > code. I mean: is anyone forcing your team (Laurent) to reformat the whole class > when editing an existing source file? I personally work in a team where not > [...] We're not forced to, but we like to have a coherent code style in our projects so that new developpers can rapidly get the whole picture without stumbling upon a particular developper's habits. Checkstyle and the formatter allow us to.
(In reply to comment #48) > Frédéric, how hard is it to add a preference? Obviously we wouldn't call it > 'Keep broken behavior' but would have to come up with a good name that reflects > the formatting preference chosen by Laurent - is that possible? If we add the > preference we can still have the new (fixed) style as default but at least give > the opportunity to back out of it by adjusting the formatter profile. I agree with that, unless adding the preference is destabilizing the code a lot (in comment 41, it sounds like the old behavior is still around). The fixed style should be the default in 3.6, but I think teams working with multiple versions of Eclipse are not that uncommon, so we should try to help them if possible. I guess Frederic is in the best position to come up with a good name for the preference that describes the new behavior (e.g. "Avoid wrapping nested expressions if possible" or "Try to keep nested expressions on one line"?).
Let me try to put my opinion on this. I believe the problem is the fix is still buggy. If I understand correctly, the goal of the fix is to group elements under the same levels as much as possible so they formatted in the same line. First example: ----------------------- public class Main { public static void main( String[] args) { foo(bar(1, 2, 3), baz(4, 5, 6, 7)); } } ----------------------- 1, 2, 3 are on the same level. So are 4, 5, 6, 7, so formatting it like: ----------------------- public class Main { public static void main( String[] args) { foo(bar(1, 2, 3), baz(4, 5, 6, 7)); } } ----------------------- is just ugly. The best solution is: ----------------------- public class Main { public static void main( String[] args) { foo(bar(1, 2, 3), baz(4, 5, 6, 7)); } } ----------------------- but it is over 40 characters. Based on this observation (elements within the same level should be on the same line whenever possible), I can comment the code from Laurent like this: ---------------------------------------------------------------- -----8<----- source.log(AcceleoParserMessages.getString( "AcceleoParser.Error.InvalidAST", oURI.lastSegment()), 0, -1); //$NON-NLS-1$ ----->8----- -----8<----- source.log( AcceleoParserMessages.getString( "AcceleoParser.Error.InvalidAST", oURI.lastSegment()), 0, -1); //$NON-NLS-1$ ----->8----- I agree that this is wrong behavior. It should be -----8<----- source.log( AcceleoParserMessages .getString("AcceleoParser.Error.InvalidAST", oURI.lastSegment()), 0, -1); //$NON-NLS-1$ ----->8----- but then the line width is too big, so old behavior is the best one. ----------------------------------------------------------------- -----8<----- AcceleoUIActivator.getDefault().getLog().log( new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages .getString("AcceleoLaunchDelegate.MissingProfileModel"))); //$NON-NLS-1$ ----->8----- -----8<----- AcceleoUIActivator .getDefault() .getLog() .log(new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages .getString("AcceleoLaunchDelegate.MissingProfileModel"))); //$NON-NLS-1$ ----->8----- In my opinion, the correct way to format this is as follows: -----8<----- AcceleoUIActivator.getDefault().getLog().log( new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages.getString( "AcceleoLaunchDelegate.MissingProfileModel"))); //$NON-NLS-1$ ----->8----- ----------------------------------------------------------------- -----8<----- AcceleoUIActivator.getDefault().getLog().log( new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages.getString( "AcceleoLaunchDelegate.ProfileModelError", new Object[] {e.getMessage(), }))); //$NON-NLS-1$ ----->8----- -----8<----- AcceleoUIActivator .getDefault() .getLog() .log(new Status( IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages .getString( "AcceleoLaunchDelegate.ProfileModelError", new Object[] {e.getMessage(), }))); //$NON-NLS-1$ ----->8----- In my opinion, this should be: -----8<----- AcceleoUIActivator.getDefault().getLog().log( new Status(IStatus.ERROR, AcceleoUIActivator.PLUGIN_ID, AcceleoUIMessages.getString( "AcceleoLaunchDelegate.ProfileModelError", new Object[] {e.getMessage(), }))); //$NON-NLS-1$ ----->8----- ----------------------------------------------------------------- -----8<----- IPath relativePath = fileEMTL.removeFileExtension().addFileExtension( IAcceleoConstants.MTL_FILE_EXTENSION).removeFirstSegments( folder.getFullPath().segmentCount()); ----->8----- -----8<----- IPath relativePath = fileEMTL.removeFileExtension() .addFileExtension(IAcceleoConstants.MTL_FILE_EXTENSION) .removeFirstSegments(folder.getFullPath().segmentCount()); ----->8----- I think in this case the new formatter get it right. ----------------------------------------------------------------- -----8<----- proposals.add(createTemplateProposal(name, offset - start.length(), start.length(), name.length(), AcceleoUIActivator.getDefault().getImage( "icons/template-editor/completion/Type.gif"), eClassifier //$NON-NLS-1$ .getName(), null, name)); ----->8----- -----8<----- proposals.add(createTemplateProposal( name, offset - start.length(), start.length(), name.length(), AcceleoUIActivator.getDefault().getImage( "icons/template-editor/completion/Type.gif"), eClassifier //$NON-NLS-1$ .getName(), null, name)); ----->8----- I think the correct way to format this is: proposals.add(createTemplateProposal(name, offset - start.length(), start.length(), name.length(), AcceleoUIActivator.getDefault() .getImage("icons/template-editor/completion/Type.gif"), eClassifier //$NON-NLS-1$ .getName() , null, name)); ----->8----- -----------------------------------------------------------------
(In reply to comment #48) (In reply to comment #56) I'll add a new preference. > I guess Frederic is in the best position to come up with a good name for the > preference that describes the new behavior (e.g. or ?). As now lot of people react on this bug (I'd rather prefer have feedbacks before I released the patch...), I'm really interested to have some inputs about the better name for this preference... Here are some initial proposals: - Markus' ones: + "Avoid wrapping nested expressions if possible" + "Try to keep nested expressions on one line" - mine: + "Allow aesthetic wrapping" (general) + "Improve message sends wrapping" (msg send specific) Be quick if you want to propose name(s) as the patch will be released for the next RC2 build (ie. tomorrow at 8:00am EDT). That means that the choice will be done before 6:00am...
(In reply to comment #52) (In reply to comment #57 As now a new preference will be added (see my previous comment), could you kindly open a new bug for the remaining issue in the new behavior? Then we'll use this new bug to still improve the formatter in this area (see also bug 310642)?
(In reply to comment #58) > (In reply to comment #48) > (In reply to comment #56) > > I'll add a new preference. Thanks, I guess I can stop trying to figure out a way to cope with this. > > > I guess Frederic is in the best position to come up with a good name for the > > preference that describes the new behavior (e.g. or ?). > > As now lot of people react on this bug (I'd rather prefer have feedbacks before > I released the patch...), I'm really interested to have some inputs about the > better name for this preference... Sorry, as I mentionned earlier, I really only figured out there was a bug such as this one after being struck (when giving a try to RC1). We can't follow all bugs :). Yet I believe you'd get a lot of reactions if asking for reviews of things as central as the JDT's formatter (Eclipse isn't a Java IDE ... but it is used as such by a vast majority of its users). > > Here are some initial proposals: > - Markus' ones: > + "Avoid wrapping nested expressions if possible" > + "Try to keep nested expressions on one line" > - mine: > + "Allow aesthetic wrapping" (general) > + "Improve message sends wrapping" (msg send specific) > > Be quick if you want to propose name(s) as the patch will be released for the > next RC2 build (ie. tomorrow at 8:00am EDT). That means that the choice will be > done before 6:00am... As mentionned above I couldn't pinpoint the new formatting rules, so I can't really propose a meaningful name here. Something like "Improve message sends wrapping" seems too specific ... wouldn't "Improve nested calls wrapping" be more explicit? Otherwise I'd go for Markus' "Try to keep nested expressions on one line" as it isn't pejorative for either "use" or "don't" as most of the other preferences are.
(In reply to comment #59) > (In reply to comment #52) > (In reply to comment #57 > > As now a new preference will be added (see my previous comment), could you > kindly open a new bug for the remaining issue in the new behavior? > > Then we'll use this new bug to still improve the formatter in this area (see > also bug 310642)? I'll try and go through all of our code base to try and nail down the samples for which the new formatting is obviously failing. Nanda's thoughts on comment 57 should help a bunch.
(In reply to comment #58) > Here are some initial proposals: > - Markus' ones: > + "Avoid wrapping nested expressions if possible" > + "Try to keep nested expressions on one line" > - mine: > + "Allow aesthetic wrapping" (general) > + "Improve message sends wrapping" (msg send specific) Markus's suggestions sound more user-friendly to me (I would prefer "Try to keep nested expressions on one line"). However, Frederic's ones are more specific and describe more precisely the behaviour of the formatter. So, how about a mix? "Improve message sends wrapping (tries to keep nested expressions on one line)"
Using words like "improve" or "aesthetic" can't be used as this bug nicely shows: everyone has a different understanding on whether the new stuff is indeed better or not. Therefore I also agree to use [ ] Try to keep nested expressions on one line
Given this bug here is fixed and verified I opened bug 313524 to track the work of adding a preference for it. Interested parties please add yourself to the cc-list of said bug.
