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

tree-edge: won't take #f as a tree-layout #49

Open
RenaissanceBug opened this issue Jan 30, 2019 · 6 comments
Open

tree-edge: won't take #f as a tree-layout #49

RenaissanceBug opened this issue Jan 30, 2019 · 6 comments

Comments

@RenaissanceBug
Copy link
Contributor

In the pict/tree-layout docs, the tree-edge function is specified as taking a tree-layout?, which in turn is defined to be #f or the output of the tree-layout function. However, the tree-edge contract given in pict/tree-layout.rkt is not sufficiently permissive to allow #f as an input:

> (tree-edge #f)
  tree-edge: contract violation
  expected: tree-layout?
  given: #f
  in: the 1st argument of
      (->* (tree-layout?)
           (#:edge-color (or/c string?
                               (is-a?/c color%)
                               (list/c byte? byte? byte?))
            #:edge-width (or/c 'unspecified real? #f))
           tree-edge?)
  contract from:
      <pkgs>/pict-lib/pict/tree-layout.rkt
  blaming: anonymous-module
   (assuming the contract is correct)
  at: <pkgs>/pict-lib/pict/tree-layout.rkt:20.10

The practical consequence is that this appears to prevent the creation of a binary-tree-layout with one or more individually customized edges.

The immediate cause is that the tree-layout? given as first input in the contract is the struct type predicate from private/layout.rkt, and not the more permissive _tree-layout? function available to client code. Making the input be _tree-layout? does fix the immediate problem and allow tree-edge to take #f; however, naive-layered and binary-tidier fail to render trees created that way:

> (define (complete d)
    (cond
      [(zero? d) #f]
      [else (define s (complete (- d 1)))
            (tree-layout (tree-edge #:edge-color "red" s) s)]))
> (naive-layered (complete 4))
. . ../../Applications/Racket v7.1/collects/racket/private/kw.rkt:1281:57: pin-line: contract violation
  expected: pict-path?
  given: #f
  in: the 4th argument of
      (->* (pict-convertible?
             pict-path?
             (-> pict? pict-path? (values number? number?))
             pict-path?
             (-> pict? pict-path? (values number? number?)))
           ((or/c #f number?)
            (or/c #f string?)
            boolean?
            #:style (or/c #f symbol?))
           pict?)
  contract from:
      <pkgs>/pict-lib/pict/private/utils.rkt
  blaming: <pkgs>/pict-lib/pict/private/main.rkt
   (assuming the contract is correct)
  at: <pkgs>/pict-lib/pict/private/utils.rkt:78.4
> (binary-tidier (complete 4))
. . Repos/pict/pict-lib/pict/private/tidier.rkt:88:9: match: no matching clause for #f

The hv-alternating function does correctly render the resulting trees.

@rfindler
Copy link
Member

If I am understanding things correctly, tree-edge makes sense only when its argument is not #f, so the fix we should have is to document it as something like (and/c tree-layout? (not/c #f)), plus adjusting the contract so the right error message prints.

The reason I say that is that there is no edge to a node that isn't drawn, and that's what #f is supposed to be. So probably you want this function:

#lang racket
(require pict/tree-layout)
(define (complete d)
  (cond
    [(zero? d) #f]
    [else (define s (complete (- d 1)))
          (tree-layout (and s (tree-edge #:edge-color "red" s)) s)]))
(naive-layered (complete 4))
(binary-tidier (complete 4))

@RenaissanceBug
Copy link
Contributor Author

Ah! OK, that makes sense.

At the time I wrote that, I was forgetting that the #f nodes weren't drawn, but remembering the line "The default node-pict (used when it is #f) is..." from tree-layout's docs. If you don't mind, in submitting the doc fix you've suggested for this, I'm going to change that "it" to node-pict, lest any other space cadet think "it" refers to the child rather than the pict.

@rfindler
Copy link
Member

Wonderful, thank you!

I also think we need to chagne the contract from (the internal) tree-layout? function to (and/c tree-layout? (not/c #f)) so that the error messages come out right and match the docs.

@rfindler
Copy link
Member

(using the external tree-layout? function in the second contract)

@RenaissanceBug
Copy link
Contributor Author

OK, commit c088bb0 has those changes.

@RenaissanceBug
Copy link
Contributor Author

And 5c4726b has the internal fn on the actual (not docs) contract.

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

No branches or pull requests

2 participants