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

Wrap field/slot references in <var class="field"> #96

Merged
merged 8 commits into from
Feb 15, 2023

Conversation

gibson042
Copy link
Contributor

Fixes #95

If/when merged, ecmarkup CSS should be updated to include something like

var.field {
  font: inherit;
  color: inherit;
}

@bakkot
Copy link
Contributor

bakkot commented Jan 24, 2023

I like the idea, but ecma-402 uses field.[[<var>]] notation in a number of places, and I think this would interfere with that (specifically, it would prevent the var part from being recognized). Do you have thoughts on fixing that?

I would lean towards only recognizing fields whose names match [0-9A-Za-z_%]+ (the % because we use %Array%-style field names for indexing the intrinsics table, which I don't love), but I'm open to other suggestions.

@gibson042
Copy link
Contributor Author

Good suggestion. Done.

@gibson042
Copy link
Contributor Author

Actually, the embedded %intrinsic%s are auto-linked, so I'm excluding them too.

@bakkot
Copy link
Contributor

bakkot commented Jan 24, 2023

I've pushed up a commit making double-brackets its own type of Node, rather than falling under Format, and marked it more precisely as containing only text, rather than arbitrary other fragments. That seemed more natural to me. If there's some reason the typings you chose are better, let me know; otherwise I'll merge this soon and get it released/upstreamed (as a major version, I guess, since it changes the interpretation of existing text).

@gibson042
Copy link
Contributor Author

I've pushed up a commit making double-brackets its own type of Node, rather than falling under Format, and marked it more precisely as containing only text, rather than arbitrary other fragments. That seemed more natural to me. If there's some reason the typings you chose are better, let me know

It's a little strange to me that double-brackets have contents: string while e.g. underscore/star/tilde/etc. have contents: FragmentNode[], but I don't object (and in fact consider the former to be a superior representation).

otherwise I'll merge this soon and get it released/upstreamed (as a major version, I guess, since it changes the interpretation of existing text).

Great! 👍

@bakkot
Copy link
Contributor

bakkot commented Jan 30, 2023

It's a little strange to me that double-brackets have contents: string while e.g. underscore/star/tilde/etc. have contents: FragmentNode[], but I don't object (and in fact consider the former to be a superior representation).

Unfortunately other format nodes aren't bare text - the parser allows you to put comments and tags inside of certain format nodes. For example, *x<span>y</span>z* is a star node with a .contents of length 5 - text, tag, text, tag, text. (FragmentNode[] is slightly too permissive, though.)

Opened #97 for the more precise types.

I'll merge this soon and get it released/upstreamed

Started doing this tonight, but it's more work than I'd realized - ecmarkup re-parses the output of ecmarkdown, and that will need to be updated.

@bakkot
Copy link
Contributor

bakkot commented Feb 1, 2023

Got this working on a local branch on top of #97 / tc39/ecmarkup#515. Only remaining issue is that it probably makes more sense not to include the brackets in the parse node, for consistency with other node types.

@gibson042
Copy link
Contributor Author

Only remaining issue is that it probably makes more sense not to include the brackets in the parse node, for consistency with other node types.

In my mind, the brackets in [[…]] are textually part of the node, while `/*/_/|/~ wrap text to indicate its proper interpretation. So I prefer the current differences between those categories, but won't object if you feel strongly about consistency between them.

@bakkot
Copy link
Contributor

bakkot commented Feb 15, 2023

In my mind, the brackets in [[…]] are textually part of the node, while `/*/_/|/~ wrap text to indicate its proper interpretation. So I prefer the current differences between those categories, but won't object if you feel strongly about consistency between them.

That's fair, but I have two reasons for prefering to exclude [[: first, that it means fewer invalid states are representable, and second, that ecmarkup already parses records and does not consider [[ part of the names of the fields in the records. So I've pushed up a commit changing to that.


Anyway, this looks good, and other editors were on board. My only remaining slight hesitation is that at some point I will want to make MOP methods behave like AOs, i.e. make it so that clicking on [[Get]] brings you to a central definition of [[Get]], and that will change the behavior for clicking on some things. But record names and MOP method names are conceptually pretty different, and there's not much use for highlighting MOP names, so I'm not really worried about that conflict.

(We do in a couple cases put invokable things inside of [[fields]], specifically with [[RegExpMatcher]], [[ReadsBytesFrom]], and [[ModifyOp]]. Not entirely sure how I want to handle those.)

And anyway perhaps instead I'll make it so that hovering over a MOP name (or even a field name?) gives you a little popup that links to the definition. Or something else entirely. I'm fine punting that problem to future me.

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.

Fields and slots should be eligible for ecmarkup interactive highlighting
2 participants