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

d2ir: Implement lazy globs and triple glob #1552

Merged
merged 21 commits into from
Sep 5, 2023

Conversation

nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Aug 16, 2023

This finishes up the globs implementation!

See tests for what I mean by lazy globs and what the triple glob does.

@nhooyr nhooyr requested a review from alixander August 16, 2023 01:42
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

failed to compile: /Users/alexanderwang/dev/d2-docs/static/bespoke-d2/triple-glob.d2:1:11: fill must be style.fill

***.style.fill: yellow
**.shape: circle
*.style.multiple: true

x: {
  y
}

layers: {
  next: {
    a
  }
}

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

applying too many times

# Create a shape `a`
a

* -> y

b
c
Screen Shot 2023-08-15 at 10 33 13 PM

@nhooyr nhooyr requested a review from alixander August 16, 2023 06:35
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

**.child

a
b
c
Screen Shot 2023-08-16 at 1 01 52 AM

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 16, 2023

Fixed.

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

i think globs need to be overridable. currently a is yellow with this script, but it should be red.

**.style.fill: yellow
**.style.fill: red

a

If a parent board has a triple glob, it should also be overrideable by globs in the current board, e.g.

***.style.fill: yellow

layers: {
  hi: {
    **.style.fill: red
    # should be red, but it's yellow right now
    a
  }
}

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 16, 2023

Certainly was the intent, not sure how that's happening.

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 16, 2023

fixed.

@nhooyr nhooyr requested a review from alixander August 16, 2023 23:52
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

did u hardcode the word layers in that last fix? i get this err again if i just change to scenarios

**.style.fill: red

scenarios: {
  b: {
    a -> b
  }
}
Screen Shot 2023-08-16 at 6 34 32 PM

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

(*** -> ***)[*].label: hi

a -> b

layers: {
  hi: {
    (*** -> ***)[*].label: bye

    scenarios: {
      b: {
        # This label is "hi", but it should be "bye"
        a -> b
      }
    }
  }
}

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 17, 2023

did u hardcode the word layers in that last fix? i get this err again if i just change to scenarios

nope using d2graph.BoardKeywords so I'm very confused by that too.

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

what do you think about this one?

(*** -> ***)[*].label: hi

# This is "hey" right now but should be "hi"?
a -> b

(*** -> ***)[*].label: hey

interpretation 1: i've defined the first glob and expect all connections to lazily apply it until it's overridden, so the current behavior is broken.

interpretation 2: globs are global. i've defined two globs of the same pattern, so the last one should win, no matter what order of statements come in between/around them, so the current behavior is correct.


i'm leaning to interpretation 1. i.e. Globs should apply backwards unless an identical pattern already has lazily applied to it.

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 17, 2023

I think number 2 is the right behavior as it's implemented now. Last one to apply always wins as that's consistent with the rest of the language. Otherwise you can't override/unset globs which isn't good. Especially if you're importing a d2 file you don't control.

Why are you leaning towards 1? Under what kind of scenario do you see this coming up where 1 is preferable?

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 17, 2023

lazily apply it until it's overridden, so the current behavior is broken.

But you did override it with the second one?

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 17, 2023

i'm leaning to interpretation 1. i.e. Globs should apply backwards unless an identical pattern already has lazily applied to it.

In this case the final glob isn't being applied backwards. It's being applied normally.

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

ok, i can see that. moving on, this one's a bug for sure:

(*** -> ***)[*].label: hi

a -> b: {
  label: bye
}
Screen Shot 2023-08-17 at 9 44 39 AM

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 17, 2023

🤦🏼 another good catch

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

looks like with filters, it's not overriding

x

*.style.opacity: 0.1
*: {
  &label: x
  style.opacity: 1
}
Screen Shot 2023-08-17 at 11 41 47 AM

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 17, 2023

First one fixed, working on this next one.

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 17, 2023

Also btw #1552 (review) was because of *** being changed to **, not because of layers -> scenarios.

And I think this next one isn't related to globs but that label isn't handled specially by filters to match the ID of a field without a label.

@alixander
Copy link
Collaborator

@nhooyr

nah i just tried this and it's the same issue

*.style.opacity: 0.1
*: {
  &label: x
  style.opacity: 1
}

x: x

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 17, 2023

Same issue there, label needs special handling. Pushed fix for *Field but I think I need to add the same exception for *Edge too. So hold on.

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 17, 2023

Fixed.

@nhooyr nhooyr requested a review from alixander August 17, 2023 21:54
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

is it easy to get globs working for imports? right now this doesn't work but i definitely see the utility

rules.d2

*.style.fill: red

index.d2

...@rules
x

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 30, 2023

Right now no unfortunately. Would require changing how imports work to merge ASTs together instead of IRs.

@alixander
Copy link
Collaborator

i think that can be another use case of ***. they cross board boundaries and file boundaries

@alixander
Copy link
Collaborator

can we encode globs into the IR? i think imports should continue just dealing with IRs, globs just need to be in there

@nhooyr
Copy link
Contributor Author

nhooyr commented Aug 30, 2023

Perhaps, I'm not sure of a good way to do it right now.

@nhooyr nhooyr force-pushed the lazy-globs branch 2 times, most recently from 303058d to 1d9f0aa Compare September 4, 2023 05:52
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

the fill isn't applying, but if I remove the x, it works. Looks like something straightforward missed?

(left is index)
Screen Shot 2023-09-04 at 11 32 53 AM

Screen Shot 2023-09-04 at 11 32 59 AM

@nhooyr nhooyr merged commit dd131f8 into terrastruct:master Sep 5, 2023
2 checks passed
@nhooyr nhooyr deleted the lazy-globs branch September 5, 2023 09:03
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.

2 participants