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

fill unspecified options with defaults for lexers and parser #1015

Closed
wants to merge 1 commit into from

Conversation

solugebefola
Copy link

@solugebefola solugebefola commented Jan 15, 2018

This ensures that if a custom Parser or Lexer is made with an options object as an argument, any unspecified options will be set by marked.defaults rather than being undefined.

One note: I ran the test suite before making changes, and there were two failures

@KostyaTretyak
Copy link
Contributor

KostyaTretyak commented Jan 17, 2018

You can use marked.setOptions() to merge your options with marked.defaults.

Your Pull Request significantly reduces performance. Just try run node test -t before and after your PR.

@Feder1co5oave
Copy link
Contributor

It's true, I had a 10% performance drop with this. Seems like mergeing 1000 times is an expensive task, compared to the actual parsing and lexing.

Additionally, the task of setting unspecified options is taken care of by the marked() function, so if you use that (as do 99% of the users) you are fine.

@solugebefola
Copy link
Author

solugebefola commented Jan 18, 2018

Thanks for the feedback. I will see if I can increase the performance while maintaining the functionality for custom Parsers and Lexers.

At the very least, maybe a note in the README about custom Parsers and Lexers.

@joshbruce joshbruce added this to the 0.5.0 - Architecture and extensibility milestone Jan 20, 2018
@joshbruce
Copy link
Member

Flagging for 0.5.0 to concentrate on fixing known issues with handling different specs...we're getting there! Very exciting.

@Feder1co5oave
Copy link
Contributor

I think the right way to fix this would be to use the solution proposed in #907.
You require('marked'), you set the options thereby instantiating the lexer and parser with those options, and you can reuse those instances on multiple inputs.
Unfortunately this is a fundamental API change that would require a major version (or minor, since we're still in 0.x?) upgrade.

@joshbruce
Copy link
Member

Definitely think it would be in the 0.5.0 minor/major camp as tagged in the #907 issue. Fix the defects for those using Marked's current APIs; then change the APIs to let people opt-in to those breaking changes without having something we know is already broken. :)

@joshbruce joshbruce removed this from the 0.5.0 - Architecture and extensibility milestone Apr 4, 2018
@joshbruce
Copy link
Member

Believe we can close this in favor of the solution by @UziTech in #1203. Feel free to reopen.

@joshbruce joshbruce closed this Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants