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

add link identifiers into emitted tags #436

Merged
merged 171 commits into from May 28, 2023
Merged

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Mar 29, 2020

This is implementation for #434. I do not like the fact that id is added for both inline and reference links, however different approaches were way more intrusive (and more difficult for newbie to implement). I have proposed multiple options in the issues maybe we can discuss to identify one that would be most appropriate.

@Gozala
Copy link
Contributor Author

Gozala commented Apr 6, 2020

Any feedback on this ?

@marcusklaas
Copy link
Collaborator

Sorry about the long radio silence. I acknowledge that your use case is a valid one, but I have my doubts about widespread its use is.

Another way you could achieve this goal without having to modify pulldown itself is to use the links source map to read the tag.

@Gozala
Copy link
Contributor Author

Gozala commented Apr 7, 2020

Sorry about the long radio silence. I acknowledge that your use case is a valid one, but I have my doubts about widespread its use is.

I do not know. However reference labels are internally parsed and saved to do resolution so it not really introducing complexity merely just exposing information that is already there.

Another way you could achieve this goal without having to modify pulldown itself is to use the links source map to read the tag.

Perhaps once #430 is fixed that would be an option.

@Gozala
Copy link
Contributor Author

Gozala commented Apr 7, 2020

@marcusklaas I think it is also worth considering issue #423 in this context. And specifically:

I think it is worth considering how currently Event::FootnoteReference(CowStr<'a>) and Tag::FootnoteDefinition(CowStr<'a>) are decoupled from each other and how one can be present without the other.

That is pretty much what I believe we need for link / image reference and their reference definitions.

It appears to me that it would make a lot more sense to have link / image reference be similar to footnote references. Instead of resolving those and concealing details like reference identifiers. In such design exposing reference identifiers is natural. It also would imply that second pass that I believe is there to do reference resolutions would be unnecessary (at least for use cases that don't require serializing markdown to html).

raphlinus and others added 6 commits April 28, 2020 19:50
Will let us publish a release for pulldown-cmark#438
Also updates some deps (bumpalo and quote were on yanked versions).
We want to say "lets us" here, not "let is us".
@alerque
Copy link
Contributor

alerque commented May 20, 2020

but I have my doubts about widespread its use is.

I have a use-case for it.

marcusklaas and others added 12 commits May 20, 2020 19:17
We would previously close lists when a line wasn't completely empty but didn't have the required indentation. Now lines that only contain whitespace are also correctly identified as empty.
It was previously only present in the *generated* test file.
This makes it easier for new contributors to understand the code by
reducing the number of abstractions that need to be learned.
This is only possible because things are now Option
Remove TreePointer in favor of Option<TreeIndex>
@dsherret
Copy link

+1 would be great to have this or provide a way in pulldown-cmark to lazily get this information out of the node when providing a range for a link.

marcusklaas and others added 3 commits July 2, 2020 16:56
The update didn't include the table header, it was right at start!
Updates dependencies, bumps version to 0.7.2
Martin1887 and others added 17 commits April 17, 2023 11:22
YAML-style and pluses-delimited metadata blocks are supported, each one
in a different `Options` flag. This is a breaking change because a new
`Tag` enum variant is added.

Fixes: pulldown-cmark#580
…attrs

feat!: allow custom headings attributes with optional value. Fixes pulldown-cmark#634
…/bumpalo-3.12.0

dependencies: bump bumpalo from 3.10.0 to 3.12.0
Fix parser bug where link label gets broken by ] in code span
Make `html` and `escape` modules optional
…-lists

Add task list with a list item's first paragraph
@Martin1887
Copy link
Collaborator

I think that enum variants get too much complicated to have unnamed fields instead of named fields, so a new pull request replicating the functionality will be implemented if this is not changed.

Thanks.

src/parse.rs Outdated Show resolved Hide resolved
src/parse.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Martin1887 Martin1887 left a comment

Choose a reason for hiding this comment

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

See comments in the code, too much complicated enum variants to have unnamed fields.

@Martin1887 Martin1887 merged commit cd20ed8 into pulldown-cmark:master May 28, 2023
1 check was pending
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