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

Proposal to allow Rule.check to identify the current rule #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrisjsewell
Copy link
Member

In markdown-it JS, via the alt list,
you could specify that a rule interrupted another block, only for certain block types.

This is a rough proposal to re-implement that feature, such that an implementation of Rule.check could identify what the current rule was.

Currently, I'm just setting a string name on the BlockState, as this was the least intrusive way (i.e. does not require a change in the signature of check). But perhaps there is a nicer way?

In markdown-it JS, via the `alt` list,
you could specify that a rule interrupted another block, only for certain block types.

This is a rough proposal to re-implement that feature,
such that an implementation of `Rule.check` could identify what the current rule was.

Currently, I'm just setting a string name on the `BlockState`, as this was the least intrusive way (i.e. does not require a change in the signature of check).
But perhaps there is a nicer way?
@chrisjsewell
Copy link
Member Author

@rlidwka
Copy link
Collaborator

rlidwka commented Jun 22, 2023

I've worked very hard to make sure alt wasn't ever necessary. :/ It's one of less intuitive things in js version (have to constantly
think that you have multiple chains, what needs to interrupt what, and the list of these chains is hardcoded and not extendable).

What's the issue with if state.node.is::<FootnoteDefinition>() that you're currently using?

Or if !state.node.is::<Paragraph>() if you need to interrupt everything but paragraphs?

@chrisjsewell
Copy link
Member Author

Hmm, let me get back to you on this one. I was playing around with https://pandoc.org/try/, with e.g.

[^a]: paragraph
[^b]: interrupt

[^c]: paragraph

    - list
[^d]: can interrupt

paragraph
[^e]: cannot interrupt

I thought there was something the markdown-it plugin was not reproducing, but I can't see what now 😬

What's the issue with if state.node.is::<FootnoteDefinition>() that you're currently using?
Or if !state.node.is::<Paragraph>() if you need to interrupt everything but paragraphs?

The main thing was that state.node.is::<XXX>() tells you what the parent node is, but it does NOT tell you what the child is that you will be interrupting. For that you would need to know that ParagraphScanner was the rule that called fo the check

@chrisjsewell
Copy link
Member Author

(mainly though, this parser implementation is looking great thanks!)

@rlidwka
Copy link
Collaborator

rlidwka commented Jun 23, 2023

but it does NOT tell you what the child is that you will be interrupting.

I think state.node should contain a child you would be interrupting (and it probably already does for lists and blockquotes?).

And the fact that it doesn't do that for Paragraph is just a bug specifically in paragraphs.

@rlidwka
Copy link
Collaborator

rlidwka commented Jul 6, 2023

I think state.node should contain a child you would be interrupting (and it probably already does for lists and blockquotes?).

I've checked the code, and it doesn't. Nevermind.

Hmm, let me get back to you on this one. I was playing around with https://pandoc.org/try/, with e.g.

So can you check if it's really necessary?

If it is, your approach seems to be good, except it should be state.test_rules_at_line::<Blockquote>() instead of state.test_rules_at_line("blockquote") (use type ids instead of strings to keep performance overhead at minimum, and avoid string collisions).

But I feel like child node should avoid knowing about the exact parent type, because it might break interoperability between two custom plugins.

@chrisjsewell
Copy link
Member Author

So can you check if it's really necessary?

sorry, I do hope to get round to this eventually 😅

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.

None yet

2 participants