seanjensengrey / kiama

Automatically exported from code.google.com/p/kiama
GNU Lesser General Public License v3.0
0 stars 0 forks source link

PrettyPrinter introduces empty space on list() with empty prefix #71

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
We use the pretty printer to also print back the AST in the syntax of our 
language. The recent change (link below) in PrettyPrinter.scala breaks our use 
of the list() pretty printer. Before, a list() with an empty prefix would work 
fine: the resulting text would start with the lparens. Now it starts with an 
empty space. (I guess the code assumes the prefix will never be empty). See 
line 329 in the link below, where <> was replaced by <+>

Not sure whether this is a bug, or a feature request for a new method providing 
a 'list' that behaves as before.

https://code.google.com/p/kiama/source/diff?spec=svn9c64590d278c061908360a5f4dfc
3398b7ecc4fb&r=b728e51c0729d349f4ce2254cc178c97ad89ad81&format=side&path=/librar
y/src/org/kiama/output/PrettyPrinter.scala&old_path=/library/src/org/kiama/outpu
t/PrettyPrinter.scala&old=60f1acb6abd751b8b66876d329a86854633767be

Original issue reported on code.google.com by msbra...@gmail.com on 13 Feb 2015 at 1:04

GoogleCodeExporter commented 9 years ago
The change you refer to was made to bring the `list` and `seq` combinators in 
line with the `any` combinator. Previously, the former two combinators didn't 
include a space after the prefix, whereas `any` did (even when formatting 
lists). I prefer to use a space for this kind of thing since I think it makes 
the output the most readable.

I think the way forward is to extract the non-prefix part of `seq` out into 
another combinator and you can use that one. That code just uses `parens` and 
`lsep` (by default) to format the argument part, which you could do yourself, 
but it would be easy to provide a combinator for it (named perhaps `arguments`).

What do you think? Would that do the trick for your use case?

Original comment by inkytonik on 16 Feb 2015 at 1:06

GoogleCodeExporter commented 9 years ago
Using the already-available parens and lsep is enough for me.
As for the presence of the space or not, normally I'd say that since there is a 
built-in to actually put in a space, then, as a user of the library, it makes 
less sense to me that in some cases the code just puts it in by itself. It took 
me some time to find it out because I never considered that the library would 
do it by itself (seemed illogical from a api standpoint, since there is <+> for 
that).

Original comment by msbra...@gmail.com on 17 Feb 2015 at 12:40

GoogleCodeExporter commented 9 years ago
I see your point, but it's not trivial to add a space here if you want one, 
since <+> combines two documents with a space between, it doesn't just append a 
space. You'd need prefix <+> empty or something like that.

Anyway, for now I've factored out the argument list pretty-printing code from 
seq into a new method call arguments.You could use it for the empty prefix case.

This is all in the latest snapshot. Thanks again for the report.

Original comment by inkytonik on 17 Feb 2015 at 10:40