Skip to content

Add support for inferring phrasing of embedded hast #74

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

Merged
merged 6 commits into from
Aug 16, 2022
Merged

Add support for inferring phrasing of embedded hast #74

merged 6 commits into from
Aug 16, 2022

Conversation

TRIAEIOU
Copy link
Contributor

@TRIAEIOU TRIAEIOU commented Aug 12, 2022

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Expand check if mdast node is phrasing content to also check node.data.tagName agains hast phrasing. This allows correct classification in conversion of hast nodes into custom mdast nodes.

Notes

  • As per earlier discussion.
  • This implementation gives priority to the hast phrasing when conflicting with mdast phrasing.

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Aug 12, 2022
@github-actions

This comment has been minimized.

@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Aug 12, 2022
lib/util/wrap.js Outdated

/**
* @param {MdastNode} node
* @returns {boolean}
Copy link
Member

Choose a reason for hiding this comment

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

You could make this a type guard btw, something along the lines of:

* @returns {node is MdastPhrasingContent}

should do it I believe? That means the cast later would no longer be needed.

This is a nice to have, not required. If it doesn’t work (well), the changes in this PR look good as is!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My JSDoc-fu is inexistent so I just implemented your change.

Copy link
Member

Choose a reason for hiding this comment

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

It’s more a TS thing, although written inside JSDoc, the grammar is TS!

TRIAEIOU and others added 3 commits August 16, 2022 15:29
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
@@ -80,7 +96,7 @@ function runs(nodes, onphrasing, onnonphrasing) {

if (phrasing(node)) {
if (!queue) queue = []
queue.push(node)
queue.push(/** @type {MdastPhrasingContent} */ (node))
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should not be needed anymore. I’ll check myself.

@@ -45,6 +45,7 @@
"hast-util-is-element": "^2.0.0",
"hast-util-to-text": "^3.0.0",
"mdast-util-phrasing": "^3.0.0",
"hast-util-phrasing": "^2.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

In unified, we prefer to keep loose ranges, if possible, so 2.0.0. It helps resolving packages by making the ranges loose. And it keeps package lock diffs cleaner.
I’ll fix this myself here, but just an FYI for the future!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks. I'm learning as I go along with npm, package.json as well as git.

Copy link
Member

Choose a reason for hiding this comment

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

no problem, I’ll try to keep that mind, and do let me know when there’s something you’re wondering about in all this!

@wooorm wooorm changed the title Expand phrasing logic with hast classification Add support for inferring phrasing of embedded hast Aug 16, 2022
@wooorm wooorm merged commit d9b2ddd into syntax-tree:main Aug 16, 2022
@github-actions

This comment has been minimized.

@wooorm wooorm added 🦋 type/enhancement This is great to have 🗄 area/interface This affects the public interface 💪 phase/solved Post is done labels Aug 16, 2022
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Aug 16, 2022
@wooorm
Copy link
Member

wooorm commented Aug 16, 2022

Thanks, released!

@TRIAEIOU TRIAEIOU deleted the phrasing-revision branch August 22, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🦋 type/enhancement This is great to have
Development

Successfully merging this pull request may close these issues.

2 participants