Community
Participate
Working Groups
The problem: There is currently no combination of settings that allow Eclipse to format this code in the following way: int someValue = new Object().toString().length(); Stream.of("one", "two", "three", "four") .filter(e -> e.length() > 3) .peek(e -> System.out.println("Filtered value: " + e)) .map(String::toUpperCase) .peek(e -> System.out.println("Mapped value: " + e)) .collect(Collectors.toList()); On the first line of code, I really do want the method calls to be chained together with no line splits. This is especially true if I chain a couple of methods like this inside of an if statement. On the second line of code, I really do want each stream call to be on it's own textual line, because despite being a single line of Java code, each textual line is a step in a process. My current solutions are to let it format the first line like the second, the second line like the first, or place @formatter: off comments in my code whenever I use the Stream API. None of these solutions are ideal. The proposed solution: The Code Formatter has the option under Line Wrapping -> Function calls -> Qualified invocations to wrap or not (or only where necessary for line length). An additional condition should be added which a user can customize, whether to split lines when the invoking instance and return type are the same type. Various builder libraries would automatically format one .withX(x) per line with this convention as well, and Stream objects do the right thing by default. This is something I would feel comfortable tackling if the maintainers of the project are okay with me adding this sort of feature.
Interesting idea, but I see one big obstacle: the formatter doesn't perform AST binding, so it doesn't know the types of variables nor return types of methods. It's not impossible to add AST binding, but it would probably affect the performance quite badly. Also, the feature can lead to some weird effects. For example, with automatic format on save turned on, if you save the code with a typo in a method name or a missing import, the formatter will not recognize the right pattern and will format the code one way, then with the problem solved it will reformat it another way. In bug 303519, the initial proposition was to add a feature so that when the number of items to be wrapped exceeds a certain threshold, the formatter switches from "wrap where necessary" to "wrap all". How does that sound? It's much easier to implement :) Have you considered using "Never join already wrapped lines"? This way you can manually fix line wrapping and it will not get reformatted.
(In reply to Mateusz Matela from comment #1) > In bug 303519, the initial proposition was to add a feature so that... The actual bug for this is bug 365264.
Thanks for the heads up about the current lack of AST binding. I do think a threshold going from "wrap where necessary" to "wrap all" (or even just adding a "wrap all where necessary" style option which would wrap one item per textual line as soon as the code line reached wrapping length) is a good solution that sounds less involved than my solution. I'd still be curious to see how much work my proposed solution requires / how much performance impact there is, but it seems like a lower priority exploration given the simpler solution. I would argue that if a typo in a method name or a missing import caused the formatting to change, that would actually be 'correct'. Great suggestion on "Never join already wrapped lines". I can't speak for other users, but the reason I don't favor this solution is that I want the code itself to determine the formatting in a predictable way. I'd prefer to have a formatter that always does the same thing, regardless of when code has too many or too few line breaks. In the existing bug you mention, it seems that the last activity on this was four years ago and there was once a patch for this. Is this something that is out in the wild somewhere or did this get shelved? It seems like a good solution, so if the only thing holding it back is someone implementing it, I'd be happy to take a swing at it.
(In reply to John Uckele from comment #3) > Thanks for the heads up about the current lack of AST binding. > I'd still be > curious to see how much work my proposed solution requires / how much > performance impact there is It's also a big change in API. Currently you just pass your code to the formatter as a String and it doesn't care about anything else (so it's portable). Now it would need a new mode that uses a java project with classpath. > In the existing bug you mention, it seems that the last activity on this was > four years ago and there was once a patch for this. Is this something that > is out in the wild somewhere or did this get shelved? It seems like a good > solution, so if the only thing holding it back is someone implementing it, > I'd be happy to take a swing at it. This was implemented in initial patches for bug 303519 which tried to modify the old formatter, so it got ditched once the formatter was rewritten from scratch. So you can have a look at their proposition for changes in API, but the actual implementation must be completely new.
I find incredible that no more developers voted for this issue. As an Eclipse user, I consider that not having a formatting configurable for fluent methods is really terrible. I addressed recently a question about that in Stackoverflow : https://stackoverflow.com/a/47867073/270371 It is a way that suits for me the most of the time but not every time. My way has a trade-off : accepting that each qualified invocation be always on a distinct line. It is maybe the missing option in the formatter configuration : indicating the threshold in terms of invocations to wrap the line instead of using 1 invocation by default. Finally it is what Mateusz Matela proposes as alternative to a full class aware formatter. I would like to implement that in Eclipse but I am not sure that this issue with so few votes will be released. I found this github https://github.com/eclipse/eclipse.jdt.core but I don't see details on the homepage on how the pull requests are prioritized.
> I would like to implement that in Eclipse but I am not > sure that this issue with so few votes will be released. if this is important to you, then you should contribute to Eclipse. If you contribute something that gets merged, it will get released. > I found this github https://github.com/eclipse/eclipse.jdt.core but > I don't see details on the homepage on how the pull requests are prioritized. as far as I know, the Eclipse JDT project can't accept contributions via Git Hub Pull Requests, but I'm sure they would be only too delighted to see your contribution via https://git.eclipse.org.
@Michael Vorburger, thanks for your answer. I will probably give it a go. About the way to improve the formatter, I thought of two things : defining the number of methods as discussed and also adding a specific annotation or comment task (such as FIXME, or TODO) to specify at which statement performs the line wrapping. The first option is interesting but not enough alone. For example, suppose I require a line wrapping as soon as the third invocation, it could produce some inconsistencies. For example this would give : myList.getFoo().stream() .map(...) .filter(...) ... but in another case : myList.stream().map(...) .filter(...) ... So, additionally being able to use a line wrapping marker would be great. It could give : // Fine with default int someValue = new Object().toString().length(); // Fine with default myList.getFoo().stream() .map(...) .filter(...) ... but in another case : // Not fine with default, so use the comment myList.stream() //WPLN .map(...) .filter(...) ... Maybe Eclipse proposes already such a thing but not found.
Maybe configurable list of regexes for method name can be used instead of type checking to find out when to start breaking line when chain is found. For example "^stream$" or "^(createB|b)uilder$" or "^assertThat$" (type checking for builder wouldn't work anyway). Therefore we would have: myList.getFoo().stream() .map(...) .filter(...) ... and also : myList.stream() .map(...) .filter(...) ... but first of all we would not have this: if( target != null && target.getName() .equals( aName ) ) { ...
Some more discussion in bug 547558. There I came with an idea of a setting for "qualified invocations where at least one argument is a lambda or a method reference". I dismissed it as too weird, but maybe it's not so bad. It could be phrased as "invocation chains with functional elements" or something like that. This would be the simplest to implement, but not very powerful. It wouldn't catch most builder APIs and there could be quite a lot of false positives and negatives, depending on programming style.
*** Bug 464194 has been marked as a duplicate of this bug. ***
i added a comment here: https://bugs.eclipse.org/bugs/show_bug.cgi?id=464194#c3 i think that is the only way really how to do this nicely We really need to know is this fully line of invocations really a "lambda" thing or not, and i think the only way to know that if there is a lambda expression somewhere in that invocation chain.. (just looking for "stream()" is really ugly) So at the moment 2 find 1 lambda expression somewhere i the chain, then we have some rules how to format it. I guess the biggest problem is where we start wrapping, is that from the first expression we did find or should we say you can start a bit earlier then like keeping 1 and the rest on the next lines.. I am fine with that we start with the one that has an expression.
(In reply to Johan Compagner from comment #11) > We really need to know is this fully line of invocations really a "lambda" > thing or not, and i think the only way to know that if there is a lambda > expression somewhere in that invocation chain.. > > (just looking for "stream()" is really ugly) Won't it work to simply look for *any* stream method invocation? (BTW I think Optional is also a good candidate for this). > So at the moment 2 find 1 lambda expression somewhere i the chain, then we > have some rules how to format it. I guess the biggest problem is where we > start wrapping, is that from the first expression we did find or should we > say you can start a bit earlier then like keeping 1 and the rest on the next > lines.. > > I am fine with that we start with the one that has an expression. I think the best would be when formatting lamdas/streams/optionals invocations could be configured independent, things like "break first lamda" and indentation could then be configured there.
(In reply to Mateusz Matela from comment #1) > Interesting idea, but I see one big obstacle: the formatter doesn't perform > AST binding, so it doesn't know the types of variables nor return types of > methods. (In reply to Mateusz Matela from comment #9) > There I came with an idea of a setting for "qualified invocations where at > least one argument is a lambda or a method reference". > I dismissed it as too weird, but maybe it's not so bad. It could be phrased > as "invocation chains with functional elements" or something like that. This > would be the simplest to implement, but not very powerful. Does it mean, formatter *can* understand "invocations where at least one argument is a lambda or a method reference" without AST binding? If so, can we have such a "simple" wrapping on lambda/method reference? Currently we format this code in an unreadable one line spaghetti: return getFactories(adaptable.getClass()).getOrDefault(adapterType, Collections.emptyList()).stream().flatMap(factory -> getFactory(factory, force).stream()).flatMap(factory -> classForFactory(factory, adapterType).map(adapterClass -> factory.getAdapter(adaptable, adapterClass)).stream()).findFirst().map(Object.class::cast).orElseGet(() -> adapterType.equals(adaptable.getClass().getName()) ? adaptable : null); Ideally formatter would produce something like this: return getFactories(adaptable.getClass()).getOrDefault(adapterType, Collections.emptyList()).stream(). flatMap(factory -> getFactory(factory, force).stream()). flatMap(factory -> classForFactory(factory, adapterType). map(adapterClass -> factory.getAdapter(adaptable, adapterClass)).stream()).findFirst(). map(Object.class::cast). orElseGet(() -> adapterType.equals(adaptable.getClass().getName()) ? adaptable : null);
Sorry to revive an old thread but I also think that something could be improved here. Since the formatter does not have access to the AST binding, it does not know that it's dealing with a Stream, a builder, an optional etc. However (correct me if I'm wrong) it should know that it is a chained invocation, i.e. calling repeatedly a method on the result of the previous call: list.stream().distinct().map(Object::toString).max(Comparator.comparing(String::length)).orElseGet(() -> defaultValue()); Could be wrapped around chained invocations as: list .stream() .distinct() .map(Object::toString) .max(Comparator.comparing(String::length)) .orElseGet(() -> defaultValue()); And similarly x.newBuilder().name("x").id(42).age(23) Could be wrapped as: x .newBuilder() .name("x") .id(42) .age(23) Here the formatter only needs to know that these are chained invocation and there could be a setting with the maximum number of invocations before wrapping kicks in
(In reply to Guillaume Toison from comment #14) > > Here the formatter only needs to know that these are chained invocation and > there could be a setting with the maximum number of invocations before > wrapping kicks in Thank for the input, so another vote for ideas from bug 365264. Any particular reasons you're not in favor of idea from comment 9? Do you think it should be a setting specific to chained method invocations, or a more general line wrapping policy (I guess it would make just as much sense for invocation arguments list, for example)? If more general, would the element count threshold be global, or a separate spinner for each line wrapping setting? Personally, I'm rather skeptical that you can use a particular threshold that would kick in for most stream/builder instances, but not for other things. Wouldn't that in practice give very similar results to settings that wrap all elements when the statement exceeds the line limit?
(In reply to Andrey Loskutov from comment #13) > Does it mean, formatter *can* understand "invocations where at least one > argument is a lambda or a method reference" without AST binding? Sorry, I missed you comment earlier. Of course, we can recognize a direct lambda or method reference in AST without bindings. We just won't recognize if there's a variable or method with type that is a functional element. > If so, can we have such a "simple" wrapping on lambda/method reference? > Ideally formatter would produce something like this: > > return getFactories(adaptable.getClass()).getOrDefault(adapterType, > Collections.emptyList()).stream(). > flatMap(factory -> getFactory(factory, force).stream()). > flatMap(factory -> classForFactory(factory, adapterType). > map(adapterClass -> factory.getAdapter(adaptable, > adapterClass)).stream()).findFirst(). > map(Object.class::cast). > orElseGet(() -> adapterType.equals(adaptable.getClass().getName()) ? > adaptable : null); If I understand you correctly, this is exactly the idea in comment 9. It would produce this kind of wrapping, except for keeping dots at line endings - that's another story :)
*** Bug 444970 has been marked as a duplicate of this bug. ***
(In reply to Mateusz Matela from comment #15) > > Thank for the input, so another vote for ideas from bug 365264. Any > particular reasons you're not in favor of idea from comment 9? > This is clearly my very personal/subjective take on it, but I feel like after 4 chained invocations the code gets harder to read on one line. I agree that this correlates with the line length but you can easily cram a few short invocations on a single line: list.stream().limit(100).distinct().toList(); One merit of this option is that it is very simple to convey what's it's going to do: wrap lines when there are more than x invocations. Clearly when some of these invocations have lambda arguments the code is even harder to read but it is also true for not lambda arguments: list.stream().limit(computeMaxSize(x)).distinct().toList(); So one relatively simple way variation would be to only count (or double count) the invocations with one or more argument(s). When that exceeds some threshold, then wrap the entire chain of invocations.
