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

NFE: add dry run capabilities to default slugger #1728

Merged
merged 6 commits into from
Jul 15, 2020
Merged

NFE: add dry run capabilities to default slugger #1728

merged 6 commits into from
Jul 15, 2020

Conversation

flouc001
Copy link
Contributor

@flouc001 flouc001 commented Jul 11, 2020

Marked version: 1.1.0

Markdown flavor: n/a

Description

This PR adds a feature that allows the developer to use the default slugger without updating the internal accumulator. I have seen a use case for this when developers are implementing their own custom renderers. For example, see this function in the TC39 temporal-proposal repository. The changes made can be summarized as:

  • Extends the default slugger with the ability to generate slug text without trigger internal communication.
  • Separates slugger responsibilities into methods for re-use.

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@vercel
Copy link

vercel bot commented Jul 11, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/markedjs/markedjs/8t7vm3sc9
✅ Preview: https://markedjs-git-fork-flouc001-nfe-dry-run-slug.markedjs.vercel.app

src/Slugger.js Outdated Show resolved Hide resolved
test/unit/marked-spec.js Outdated Show resolved Hide resolved
@flouc001
Copy link
Contributor Author

@UziTech I've made the changes you requested, the only thing I wasn't sure on was how you wanted the option documented in the code. I've added basic JSDoc but I can't see an example of it being used similarly anywhere else in the codebase so happy to take it out 👍

@UziTech
Copy link
Member

UziTech commented Jul 14, 2020

JSDoc works fine. Documentation in the code could definitely be better.

@joshbruce
Copy link
Member

joshbruce commented Jul 14, 2020

Not that it has too and it would be good to know.

Does this affect special characters and asian character sets as that's been a topic: #664

Or, is it specifically giving the ability for a developer to intervene and verify themselves??

In either case, I will approve but not merge as this one feels a bit deeper than my understanding at present.

@flouc001
Copy link
Contributor Author

flouc001 commented Jul 14, 2020

@joshbruce

Does this affect special characters and asian character sets as that's been a topic: #664

This code should not affect how the heading id is actually generated so there will be no deviation from default behaviour on that front.

Or, is it specifically giving the ability for a developer to intervene and verify themselves??

Exactly, this option is all about giving control to the developer who, for whatever reason, may want to generate heading ids without incrementing the Slugger's seen counter. At the moment there is no way to generate the heading id without causing the Slugger to mutate that internal counter.

docs/USING_PRO.md Outdated Show resolved Hide resolved
test/unit/marked-spec.js Outdated Show resolved Hide resolved
Co-authored-by: Steven <steven@ceriously.com>
Co-authored-by: Tony Brix <tony@brix.ninja>
@flouc001
Copy link
Contributor Author

@styfle I've added the test you recommended and committed all outstanding suggestions 👍

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Thanks!

@styfle styfle merged commit ba13064 into markedjs:master Jul 15, 2020
@flouc001 flouc001 deleted the nfe/dry-run-slug branch July 15, 2020 15:25
@UziTech UziTech mentioned this pull request Sep 27, 2020
12 tasks
@github-actions
Copy link

🎉 This PR is included in version 1.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants