Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proper reprs for Hy models #1360

Merged
merged 7 commits into from
Sep 18, 2017
Merged

proper reprs for Hy models #1360

merged 7 commits into from
Sep 18, 2017

Conversation

gilch
Copy link
Member

@gilch gilch commented Aug 3, 2017

Closes #914.

This will make development of macros sooo much easier, since we can tell exactly what we're looking at in the repl with macroexpand or any other manipulations of Hy models.

The reprs for HyList and derivatives will use two-space indents, so even deeply nested structures are readable in the repl.

This also makes the reprs valid Python code to reconstruct the hytree in most cases, as is preferred in Python. This will improve Python interop somewhat, since if a Hy model leaks through, the error message will make sense as a Hy model, instead of looking like Hy code that could be confused for some kind of Python display.

@gilch gilch force-pushed the model-repr branch 3 times, most recently from a61e090 to 30dd07a Compare August 3, 2017 23:23
@gilch
Copy link
Member Author

gilch commented Aug 3, 2017

This breaks hy-repr for obvious reasons. I'm xfailing for now. This will have to be corrected, but not necessarily in this PR. (Experimental features shouldn't hold up core fixes, and furthermore, reprs are subject to change even between Python versions so should probably not be relied upon for correct behavior.) That test file is also not indented properly. I had to edit it as plaintext mode instead of as a Hy file to avoid excessive changes.

@Kodiologist
Copy link
Member

Nice!

I don't think hy-repr should be hard to fix. But first let's see some tests for the change itself.

@refi64
Copy link
Contributor

refi64 commented Aug 4, 2017

Although I like the idea, is there a reason why the HyCons repr is totally different in style from the others (e.g. <xxx yyy> vs xxx(yyy))?

@gilch
Copy link
Member Author

gilch commented Aug 4, 2017

@kirbyfan64, this is so nested conses are still readable as dotted lists, for example

=> (cons 1 (cons 2 3))
from hy.core.language import cons
cons(1, cons(2, 3))
<HyCons (HyInteger(1) HyInteger(2) . HyInteger(3))>
=> '(1 2 . 3)
from hy import HyCons, HyInteger
HyCons(HyInteger(1), HyCons(HyInteger(2), HyInteger(3)))
<HyCons (HyInteger(1) HyInteger(2) . HyInteger(3))>

Python reprs that are known to not round trip are conventionally contained in <>, for example:

>>> object()
<object object at 0x000002513237A770>

Note also, that cons is automatically converting 1 to HyInteger(1). I thought it was more important to show what it actually contains, rather than how you'd call it, like cons(1, ...). Although cons(HyInteger(1), ...) should actually be equivalent.

And furthermore, HyCons isn't autoimported like the other models. We're getting from hy.core.language import cons instead. So should it be like HyCons(foo, bar), or cons(foo, bar)? I'd pick the former, but this won't work in the Hy repl as (HyCons foo bar) after autoimport like all the other models do.

That said, I don't have very strong feelings about keeping this version of the HyCons repr. I'd consider a better version, but other things might have to change too. How would you want it to work?

@refi64
Copy link
Contributor

refi64 commented Aug 4, 2017

Touche.

@gilch
Copy link
Member Author

gilch commented Aug 19, 2017

@kirbyfan64, Thinking about HyCons some more, I'm not really satisfied.

<HyCons (HyInteger(1) HyInteger(2) . HyInteger(3))>

--should probably be spread over multiple lines like the other compound models. Maybe like this.

<HyCons (
  HyInteger(1)
  HyInteger(2)
. HyInteger(3))>

If we want it to round trip, then HyCons should be in core (probably all of the Hy models should #1045, but we should rethink autoimports altogether #434) then--

HyCons(HyInteger(1), HyCons(HyInteger(2), HyCons(HyInteger(3), HyInteger(4))))

--is the most obvious. But we'd want that spread over multiple lines too, to be consistent with the reprs from the other models.

HyCons(HyInteger(1),
  HyCons(HyInteger(2),
    HyCons(HyInteger(3),
      HyCons(HyInteger(4),
        HyInteger(5)))

But I'm not sure that I like the indent for a single list. So we could just not indent conses.

HyCons(HyInteger(1),
HyCons(HyInteger(2),
HyCons(HyInteger(3),
HyCons(HyInteger(4),
HyInteger(5)))))

But how should any of that look with a tree structure made from conses? Something like--

((1 . 2) . ((3 . 4) . (5 . 6)))

Hy currently renders it like--

((1 . 2) (3 . 4) 5 . 6)

--and Common Lisp too. Notice how we lost dots, because it's a valid list prefix when the cdr is also a cons cell.

This PR currently renders it like--

<HyCons (<HyCons (HyInteger(1) . HyInteger(2))> <HyCons (HyInteger(3) . HyInteger(4))> HyInteger(5) . HyInteger(6))>

--which is too long to really read. If we spread it over lines it's better. But is it good enough?

<HyCons (
  <HyCons (
    HyInteger(1)
  . HyInteger(2))>
  <HyCons (
    HyInteger(3)
  . HyInteger(4))>
  HyInteger(5)
. HyInteger(6))>

The round-trip version over lines without indents--

HyCons(HyCons(HyInteger(1),
HyInteger(2)),
HyCons(HyCons(HyInteger(3),
HyInteger(4)),
HyCons(HyInteger(5),
HyInteger(6))))

--is hard to read.

With indents--

HyCons(HyCons(HyInteger(1),
    HyInteger(2)),
  HyCons(HyCons(HyInteger(3),
      HyInteger(4)),
    HyCons(HyInteger(5),
      HyInteger(6))))

This seems less bad. It would be clearer if the cars had their own line though.

HyCons(
  HyCons(
    HyInteger(1),
    HyInteger(2)),
  HyCons(
    HyCons(
      HyInteger(3),
      HyInteger(4)),
    HyCons(
      HyInteger(5),
      HyInteger(6))))

It's much easier to see the structure this way. But look what that does to long lists.

HyCons(
  HyInteger(1),
  HyCons(
    HyInteger(2),
    HyCons(
      HyInteger(3),
      HyCons(
        HyInteger(4),
        HyInteger(5)))

Maybe this is okay. And it round trips (given HyCons). But the <> version spread over lines handled this much better.

<HyCons (
  HyInteger(1)
  HyInteger(2)
  HyInteger(3)
  HyInteger(4)
. HyInteger(5))>

Which do you prefer? Or do you have a better idea?

@refi64
Copy link
Contributor

refi64 commented Aug 19, 2017

IMO the last is the best (which is basically the current version but with indents, right?).

@gilch
Copy link
Member Author

gilch commented Aug 20, 2017

I redid the HyCons repr to work like the last example. @kirbyfan64 see if you like that version. If that's good, we'll still need tests and news/docs.

@refi64
Copy link
Contributor

refi64 commented Aug 25, 2017

Looks great to me!

@gilch gilch force-pushed the model-repr branch 2 times, most recently from 3c82857 to 4568a34 Compare August 26, 2017 22:20
@gilch
Copy link
Member Author

gilch commented Aug 26, 2017

I colorized the reprs of the compound models to make deeply nested structures more readable. Try it out.

Python reprs are not usually colorized like this. They're not usually indented either. evaling a colored repr is not going to work, but a manual copy/paste would. You can turn off clint's coloring by setting the clint.textui.colored.DISABLE-COLOR flag to True. clint also has a function to strip the color tags from a string. Either way would let us eval one of these. I don't see users doing this much, but I'd do it in the tests to make sure they round trip. Except for HyCons, that one doesn't work even without the color.

I feel like I'm breaking convention a bit here. Most Python reprs are not colored, are not indented, and have no newlines. But it's much closer to normal Python than the Hy-like reprs we had before, to the point that you could manually copy/paste into a Python repl and usually expect it to work.

If this is too much trouble, we could have a module flag in Hy models to turn off coloring just for the models (if we turn it off in clint, errors don't get colored either). Or we could render them without color, indentation (or newlines) by default and have a separate repr-output-fn to indent and colorize them.

I also noticed that the list* method will build nested conses. I thought we could use that in the repr to avoid the deep nesting problem of the round-tripping version of the HyCons repr. Compare

HyCons(
  1,
  HyCons(
    2,
    HyCons(
      3,
      4)))
list*(
  1,
  2,
  3,
  4)

The problem is, that won't round trip either, because list* is not a valid Python identifier. Like I said before, other things might have to change.

One option is to give the HyCons class a static factory method that does the same thing as list*. (We could then implement list* in terms of that.) Then we could repr it like.

HyCons.list(
  1,
  2,
  3,
  4)

@Kodiologist
Copy link
Member

Pretty cool. It might make sense to separate the pretty-printing code from the __repr__ code. Python's native pprint might be useful.

@gilch
Copy link
Member Author

gilch commented Aug 28, 2017

Python's native pprint might be useful.

I don't see how. Python's native pprint only works on certain Python collection types as special cases. It's not designed to be extensible at all.

It might make sense to separate the pretty-printing code from the repr code.

A mechanism to disable or enable the coloring and pretty printing would make sense. This is a great use case for a dynamic var. hylang/hyrule#51. Until that's implemented, a module-level flag would have to do. But the way the reprs are built, separating them altogether (without adding a lot of duplication) seems harder. It's much easier to add the pretty printing when the string is built than it is after, which would be error-prone. Starting with a pretty string and stripping the color works pretty well, but there might be edge cases where it strips too much. Stripping indents and newlines seems even more error-prone.

We could give Hy models a __str__ method (except for HyString and friends), which (by convention) doesn't need to even try to be evalable. So it might make more sense to render a str to look more like Hy code than like Python. __str__ is what print uses instead of __repr__. (But, if you don't override it, object.__str__ will fall back to self.__repr__.) These could also be colored and pretty-printed without much issue.

@Kodiologist
Copy link
Member

Okay, then, are we ready for tests?

@gilch
Copy link
Member Author

gilch commented Sep 6, 2017

The hy.models.PRETTY flag now controls if compound Hy model reprs are pretty or a more typical flat, colorless Python repr. Their str will be pretty in either case. (__str__ always sets PRETTY to true while generating its output, so nested compound Hy model reprs will also be pretty.) I'm not sure if PRETTY should be on or off by default, but I'm leaning towards on. It doesn't seem likely to cause problems for most users most of the time, but they can turn it off if they need to.

I'll need to mention the PRETTY flag in the docs.

@gilch gilch force-pushed the model-repr branch 2 times, most recently from 9e6cfce to e6ef61f Compare September 7, 2017 05:07
@gilch
Copy link
Member Author

gilch commented Sep 7, 2017

Okay, there are docs and tests. Do these tests look sufficient?

It should probably have a NEWS entry too, but I'll save that for last after the rest has approval, since it's likely to conflict with other PRs.

@Kodiologist Kodiologist force-pushed the model-repr branch 2 times, most recently from 82a20f7 to 435ec2f Compare September 7, 2017 18:45
@Kodiologist
Copy link
Member

Kodiologist commented Sep 7, 2017

hy-repr should be working now. (I amended the first commit and removed the xfail commit.)

HyList([
HyFloat(1.0)]),
HyDict([
HyFloat(1.0) # odd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do these comments with the text "odd" mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means there's an odd number of elements in your HyDict. This is allowed, since a HyDict is really a list, but it's a mistake if your macro expands to this, because a dict is built from pairs. So I wanted to make it very visible for debugging purposes.

Note also that # odd only shows up when PRETTY is on.

You might legitimately have an odd number in an intermediate stage of the macroexpansion though. So I don't want to add error checking to the HyDict methods to prevent that state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting. That makes sense.

@Kodiologist
Copy link
Member

I made two edits: I added the name clean in the documentation about Clint, and I added the comment "str should be pretty, even under pretty(False)" to explain a test.

@gilch gilch mentioned this pull request Sep 12, 2017
@refi64
Copy link
Contributor

refi64 commented Sep 15, 2017

After playing around for a bit, there's only one thing I find odd:

=> '#{1 2 3 {4 5} [6 7]}
HySet([
  HyInteger(1),
  HyInteger(2),
  HyInteger(3),
  HyDict([
    HyInteger(4), HyInteger(5),]),
  HyList([
    HyInteger(6),
    HyInteger(7)])])
=> 

Why is there a trailing comma only on HyDict? My OCD is saying it's kinda inconsistent...

@gilch
Copy link
Member Author

gilch commented Sep 15, 2017

Why is there a trailing comma only on HyDict?

The short answer is, "it was easier to implement that way". Would you prefer that I add a trailing comma to everything, or remove it from HyDict? The former is probably a little easier, but the latter is more consistent with Python's reprs of the things we're modeling.

@refi64
Copy link
Contributor

refi64 commented Sep 15, 2017

I mean, I'd prefer the latter, provided it's not too difficult.

@Kodiologist
Copy link
Member

Yeah, it seems best not to have trailing commas in reprs.

@gilch
Copy link
Member Author

gilch commented Sep 18, 2017

Okay, that wasn't too hard. How's that?

@refi64 refi64 merged commit db21092 into hylang:master Sep 18, 2017
@Kodiologist Kodiologist mentioned this pull request Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a pretty print function to compose with macroexpand
3 participants