Ugly output

[tombentley]

I tried for format https://github.com/ceylon/ceylon.language/blob/0f144124206213e2151c4a9a426329649eaea2b4/src/ceylon/language/meta/model/ClassOrInterface.ceylon with ceylon format --maxLineLength=100 src/ceylon/language/meta/model/ClassOrInterface.ceylon and all got some very ugly output: https://gist.github.com/tombentley/74c8ce5e817cf57c9e0d

[lucaswerkmeister]

To clarify what exactly the bug is:

• Many doc comments are pulled to the end of the preceding line:

    shared formal actual ClassOrInterfaceDeclaration declaration; "The extended closed type for this class or interface. Note that the [[Anything|ceylon.language::Anything]] type
                                                                   has no extended type since it is the top of the type hierarchy."
    shared formal ClassModel<>? extendedType;

  This is certainly a bug.

• Some annotations are not wrapped for some reason:

    throws (`class IncompatibleTypeException`, "If the specified `Container`, `Type` or `Arguments` type arguments are not compatible with the actual result, 
                                                or if the corresponding member is not a `MemberClass`.")

  should probably be

    throws (`class IncompatibleTypeException`,
        "If the specified `Container`, `Type` or `Arguments` type arguments are not compatible with the actual result, 
         or if the corresponding member is not a `MemberClass`."
    )

  This also looks like a bug.

• Even the wrapped annotations look kinda ugly:

    throws (`class TypeApplicationException`,
        "If the specified closed type argument values are not compatible with the actual result's type parameters."
    )

  should perhaps be

    throws (`class TypeApplicationException`,
        "If the specified closed type argument values are not compatible with the actual result's type parameters.")

  or

    throws (
        `class TypeApplicationException`,
        "If the specified closed type argument values are not compatible with the actual result's type parameters."
    )

  but that’s #11, not a bug.

• The blank lines between the last couple of methods got removed. This is also a bug (probably the same one as the first issue).
• Anything else I missed?

[lucaswerkmeister]

As it turns out, the first problem (annotations being pulled up) is due to a special case in DefaultLineBreaks for multi-line tokens, so the last problem (blank lines between methods removed) can’t be the same problem, since those don’t have multi-line tokens.

[lucaswerkmeister]

However, the underlying problem is that DefaultLineBreaks barely uses the existing LineBreaks (only as fallbacks when it can’t do better), which is wrong. Pre-existing LineBreaks should always be preferred to new ones. So the fix will probably still fix both problems at once.

[lucaswerkmeister]

Should be much better now, doc comments being pulled up and lines between methods being removed are both fixed now. See gist.

Remaining differences:

• Some annotations are still not wrapped, and some are still wrapped weirdly, as in the previous comment. I’ll have to look into that.
• The formatter doesn’t think blank lines between imports are okay. The thing is, there’s no way to specify that zero to two line breaks are okay, but the default if there’s no token stream is one line break. So I think that’ll stay that way.
• The formatter places a line break after sealed. I think that’s wrong, sealed should be one of the default inline annotations.
• Indentation of the of, satisfies and given is different. I side with the formatter – AFAIK, pre-indent by two levels (not one) is the Ceylon convention.
• The formatter doesn’t want spaces between type parameters and arguments (<Type, Nothing> is turned into <Type,Nothing>). This is something I’ve been in doubts about for a while… comments? I’m open for changing this, but of course it means that a bunch of now well-formatted code will suddenly be no longer well-formatted, which is a bit icky.

• However, the formatter does want spaces between type parameter and default argument, and turns

  <Container=Nothing, Type=Anything, Arguments=Nothing>

  into

  <Container = Nothing,Type = Anything,Arguments = Nothing>

  This, in combination with the above, is complete bullshit, since it moves a type argument closer to the next type parameter than the one it actually belongs to. Definitely needs to be fixed.

• The formatter puts a space between annotation name and parameter list, turning all the throws(…) into throws (…). Again, AFAIK this is Ceylon convention.
• The formatter doesn’t like two line breaks at the end of a file – I agree, one is quite enough, thank you very much.

Comments? (Not necessarily just @tombentley, everyone can pitch in!)

[gavinking]

• Spaces after commas in type arg/parameter lists should be configurable.
• Most days I prefer spaces before annotation arg lists, but perhaps it should also be configgable.

[lucaswerkmeister]

Actually, both of these are configurable: spaceAfterTypeArgOrParamListComma and spaceBeforeAnnotationPositionalArgumentList. I guess for the type argument problem, spacing around the = also needs to be configurable and/or depend on spaceAfterTypeArgOrParamListComma.

On 29.09.2015 00:21, Gavin King wrote:

• Spaces after commas in type arg/parameter lists should be configurable.
• Most days I prefer spaces before annotation arg lists, but perhaps it should also be configgable.

— Reply to this email directly or view it on GitHub https://github.com/ceylon/ceylon.formatter/issues/115#issuecomment-143890545.

[tombentley]

That is much better @lucaswerkmeister. One thing I would say (though I'm not sure if it's in scope or part of the more general line break problem) is that

throws (`class IncompatibleTypeException`, "If the specified `Container`, `Type` or `Arguments` type arguments are not compatible with the actual result, 
                                            or if the corresponding member is not a `MemberClass`.")

is a bit of a dogs breakfast, and I'd much rather see the parameter list broken than the doc string, even if the doc string gets nicely indented.

[lucaswerkmeister]

• The formatter doesn’t think blank lines between imports are okay. The thing is, there’s no way to specify that zero to two line breaks are okay, but the default if there’s no token stream is one line break. So I think that’ll stay that way.

Turns out, that’s rubbish, it’s totally possible. Fixed in #119.

[lucaswerkmeister]

So the problem with the annotation string line breaking is that currently, there’s just no way for a LineBreakStrategy to signal that a line break needs to happen before a multi-line string. Long ago, to make multi-line strings work at all, I added a gross hacky special case to the return value of lineBreakLocation, which changes its meaning if the token on that location is multi-line. A special case for such tokens is indeed necessary – you need to signal that a line ends there, even though there’s no explicit line break – but it needs to be done differently, so that an explicit new line break can also be signaled.

I pushed a partial improvement of this on branch wip-115, where the return value is now interpreted differently, in what’s hopefully a more reasonable manner. However, it’s not done yet, and DefaultLineBreaks doesn’t yet break these lines correctly. I won’t have time to work on this until next Thursday (2015-10-15).

[lucaswerkmeister]

Alright, that took a while, but line breaks around multi-line string literals should now fully work.

[lucaswerkmeister]

Another gist with the differences.

• wontfix:
  • space before annotation param list
  • two indentation levels before of, satisfies, given etc.
  • no blank line at the end
  • spaces around = sign of import aliases – is configurable
  • ugly line breaks in type parameter/argument lists – a better line break strategy would break at commas, not = signs, but that’s #11, and after how much ickiness just the simplest line break strategy turned out to be due to multi-line literals, I’m not going to touch that any time soon
• willfix:
  • bad spacing around type parameters
  • sealed forces line break

[lucaswerkmeister]

• sealed forces line break

✔ done in 7004a3d

• bad spacing around type parameters

I’ve settled on splitting up spaceAfterTypeArgOrParamListComma into spaceAfterTypeParamListComma and spaceAfterTypeArgListComma and adding another option spaceAroundTypeParamListEqualsSign. These allow you to separately control:

• whether you want to have a space after a comma in a type parameter list – defaults to true, because otherwise variances look ugly, and type parameters with default type arguments can get long
• whether you want to have a space after a comma in a type argument list – defaults to false, for more compact code
• whether you want to have spaces around the equals sign for a default type argument – defaults to false, also for more compact code

That is, the default now produces

void foo<Param1=Default1, Param2=Default2>() {}
foo<Arg1,Arg2>();

whereas it previously produced

void foo<Param1 = Default1,Param2=Default2>() {}
foo<Arg1,Arg2>();

which you could only change to

void foo<Param1 = Default1, Param2 = Default2>() {}
foo<Arg1, Arg2>();

However, these option changes require matching changes in the IDE preferences code, so I haven’t pushed them yet.

[lucaswerkmeister]

Spacing around type params now also ✔ done in 10e09d695361860cdf4401a4fb8c55355504fd7f.

Note: the old option is still accepted (…TypeArgOrParamList… sets both …TypeArgList… and …TypeParamList…), so you don’t need to manually update your config files.

[lucaswerkmeister]

Fixed a few more things. Final comparison: gist.

I’m happy with the formatted code now, so I’m going to close this issue. @tombentley feel free to reopen it or open another issue if there’s something still wrong with it.