Replace Term::deep_repr by the pretty printer #1262
Conversation
vkleen
force-pushed
the
pretty_deep_repr
branch
from
ee1f831
to
9521382
Compare
There was a problem hiding this comment.
Sounds better, but I think we had a discussion back then about what to do about e.g. functions. Where you're defining a function in the REPL, it's customary (as it, in other languages) to not re-print again the whole code of the function. Might be the same for documentation and metadata in general (I guess in the REPL you really want to print values, not the fuss around).
IIRC, one proposal was to implement Pretty on a more elaborated type, basically isomoprhic to (Term, PrettyConfig), with PrettyConfig controls the options of the pretty printer. That being said, we can consider that in a second step.
| @@ -1734,7 +1701,6 @@ impl From<Term> for RichTerm { | |||
| impl std::fmt::Display for RichTerm { | |||
| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | |||
| use crate::pretty::*; | |||
| use pretty::BoxAllocator; | |||
There was a problem hiding this comment.
Any reason to move this specific use from the local function to the top-level? We don't really have consistent rules right now about where to place use statements, but I vaguely intuit that it makes more sense to keep it local if it's very specific to the current function, and doesn't have much to do with the rest of the module, which seems to be the case here.
There was a problem hiding this comment.
That's a very good question. It might have happened during a previous iteration, but right now there's no good reason. I think I'll revert this part.
|
I've made a tracking issue for making the pretty printer configurable at #1349. |
We previously used a bespoke formatting algorithm for `Type`. I replaced the analogous code for `Term`s by the pretty printer in #1262 but we were worried about some questionable code for contract error reporting before doing the same for types. Namely, at some point it relied on hard coded string offsets for pointing at parts of types that were inferred by Nickel and consequently had no `TermPos`. In #1229 we ripped out that code and replaced it by reparsing the pretty printer output when necessary. Incidentally, this change also fixes some terms being truncated when formatted. For example, previously ``` SomeUserDefinedContract "that" "takes" "many" "arguments" ``` would be printed as `SomeUserDefinedContract "that" …`. This is somewhat useful to prevent huge screenfuls of error messages sometimes, but it makes the `Display` implementation useless for other natural purposes.
) * Use the pretty printer in the `Display` implementation for `Type` We previously used a bespoke formatting algorithm for `Type`. I replaced the analogous code for `Term`s by the pretty printer in #1262 but we were worried about some questionable code for contract error reporting before doing the same for types. Namely, at some point it relied on hard coded string offsets for pointing at parts of types that were inferred by Nickel and consequently had no `TermPos`. In #1229 we ripped out that code and replaced it by reparsing the pretty printer output when necessary. Incidentally, this change also fixes some terms being truncated when formatted. For example, previously ``` SomeUserDefinedContract "that" "takes" "many" "arguments" ``` would be printed as `SomeUserDefinedContract "that" …`. This is somewhat useful to prevent huge screenfuls of error messages sometimes, but it makes the `Display` implementation useless for other natural purposes. * Revert a useless change to snapshot test inputs * Factor out calling the pretty printer for `Display`
This PR gets rid of
Term::deep_reprand replaces all calls to it by invocations of the pretty printer. Incidentally, it ensures that quotes and newlines in strings are correctly escaped when printing evaluation results.As a bonus, the pretty printer formats records with many fields on multiple lines in the REPL if they don't fit into 80 columns.
Closes #1257