[m-rev.] for post-commit review: pretty_printer.m
Julien Fischer
jfischer at opturion.com
Sun Aug 7 15:40:11 AEST 2016
Hi Zoltan,
On Sat, 6 Aug 2016, Zoltan Somogyi wrote:
> What should be reviewed is not so much the diff, as the new contents
> of pretty_printer.m. I am after feedback mostly on the new
> NOTES_TO_IMPLEMENTORS I added. Acting on those would break
> compatibility with callers (which this diff does NOT do), but the results
> may be worth it.
I think it's ok to break backwards compatibility in the case of this
module.
> + % NOTE_TO_IMPLEMENTORS: XXX The name of this predicate is wrong.
> + % NOTE_TO_IMPLEMENTORS: This name should be attached to the predicate
> + % NOTE_TO_IMPLEMENTORS: that is currently named write_doc/4; THIS predicate
> + % NOTE_TO_IMPLEMENTORS: should be named something like write_doc_general.
> + % NOTE_TO_IMPLEMENTORS: Note that "stream" here has two related but NOT
> + % NOTE_TO_IMPLEMENTORS: IDENTICAL meanings (the stream typeclass, and
> + % NOTE_TO_IMPLEMENTORS: io.output_stream), and thus using it for this name
> + % NOTE_TO_IMPLEMENTORS: is more confusing than helpful.
I suggest renaming write_doc_to_stream/9 to put_doc/9, since that name
is consistent with the way predicates that write to string writer
streams are named in other modules (e.g. stream.string_writer). It
should also be sufficiently distinct from write_doc/4.
Julien.
More information about the reviews
mailing list