Nested method chains also adhere to column limit #123
Conversation
Generate changelog in
|
| return optional.orElseGet( | ||
| (String) () -> bar(Optional.of("some thing that is very very very looooooong")).get()); | ||
| return optional.orElseGet((String) () -> | ||
| bar(Optional.of("some thing that is very very very looooooong")).get()); |
There was a problem hiding this comment.
This changed beause it doesn't allow the dot in the bar(...).get() expression to exceed 80 chars anymore. It now ends up at exactly 80 chars, and to prove that, if I add another character to the string, then the .get() gets pushed onto the next line, as in:
return optional.orElseGet((String) () -> bar(Optional.of("some thing that is very very very loooooooong"))
.get());
| .getActualTypeArguments()[0]; | ||
| Class<T> tClass = (Class<T>) | ||
| verifyNotNull((ParameterizedType) getClass().getGenericSuperclass()) | ||
| .getActualTypeArguments()[0]; |
There was a problem hiding this comment.
Unclear if we want to be so strict with all methods, or if we should exempt non-side-effect-sounding methods like those that start with get[A-Z0-9]. @iamdanfox thoughts?
| if (doc instanceof Break && ((Break) doc).hasColumnLimit()) { | ||
| columnBeforeLastBreak = column; | ||
| } else if (doc instanceof Level) { | ||
| // Levels might have nested levels that have a 'columnLimitBeforeLastBreak' set, so recurse. |
There was a problem hiding this comment.
This recursion seems like it could end up being pretty expensive
There was a problem hiding this comment.
the integration tests take about 15s with this block commented out, and 20s with it as it is, so I think this is fine. (it's pretty much just the extremely degenerate test case that suffers)
| Level innerLevel = (Level) doc; | ||
| Optional<Integer> newWidth = innerLevel.tryToFitOnOneLine(maxWidth, newState, innerLevel.getDocs()); | ||
| if (!newWidth.isPresent()) { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
why do we return early here?
There was a problem hiding this comment.
It indicates that the innerLevel was not inlineable.
Currently, the only reason for that to happen is that it had a non-prefix method call (i.e. not something like foo.bar().stream()) that occurs after the column limit.
Once this happens it's no point trying to one line the parent level anymore.
|
👍 |
|
Released 0.3.11 |
Method chains in initializers may once again be laid onto a single next line, if they are short enough. This fixes a regression from #123 (0.3.11) where the column limit for method chains would prevent us from attempting to put the initializer on the 2nd line first and see if that works.
Before this PR
#70 (and #71) failed to account for more complex expressions that contained a nested chain of method calls.
An example where it didn't do the right thing is this test.
After this PR
==COMMIT_MSG==
Nested method chains now also adhere to column limit.
Also, we've made the logic a bit more specific in terms of what are method chains.
We now only apply this restricted column limit to breaks that come before actual method calls, not field accesses or parts of a fully qualified class name.
==COMMIT_MSG==
Possible downsides?