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

Improvements, extensions and some fixes #11

Merged
merged 12 commits into from
May 14, 2012
Merged

Improvements, extensions and some fixes #11

merged 12 commits into from
May 14, 2012

Conversation

mildsunrise
Copy link
Collaborator

Great work with Robotskirt, let's make it more complete!
And it's now even faster (it allows Renderer to be used multiple times)!
See the benchmark.

New features

  1. A class named Renderer has been created which wraps a mkd_renderer.
  2. Another class HtmlRenderer (which inherits from the above) wraps the standard (X)HTML renderer bundled with Sundown.
  3. The render functions (toHtml and toHtmlSync) now require a Renderer as first argument.
    Thus, they've been renamed to markdown and markdownSync.
  4. The functions now check the types of their arguments (prevents crashes when passing null).
  5. The extension flags have been exposed as module properties:
    EXT_AUTOLINK
    EXT_FENCED_CODE
    EXT_LAX_HTML_BLOCKS
    EXT_TABLES
    EXT_NO_INTRA_EMPHASIS
    EXT_SPACE_HEADERS
    EXT_STRIKETHROUGH
  6. The renderer functions now accept extension flags as the (optional) last argument.

Other optimizations

  • Use the NODE_MODULE macro.
  • The output buffer is directly converted to a JS Buffer instead of copying its contents.
  • Use RAII to make sure the mkd_renderer is always deleted, no matter what happens.
  • Those improvements break compatibility, so bumping version to 1.0.0.
  • Keep documentation, examples & benchmarks up-to-date.
  • Extend / document new features on README
  • Update benchmark to call the right function.

 - Expose class Renderer (opaque, for now)
 - Expose factory function htmlRenderer(flags),
   which creates and returns a standard html Renderer.
 - Rename toHtml and toHtmlSync to
   markdown(...) and markdownSync(...), because
   now they take a generic Renderer as argument.
 - Output buffer is passed in directly.
 - Use delete instead of free(...).
 - Use finally to make sure the buffers are
   deallocated, no matter what happens.
 - Some formatting.
 - mkd_renderer is now wrapped
 - stdHtmlRenderer now has its own HtmlRenderer class
 - add try {} catch {} in markdown() to avoid crashes
 - use ObjectWrapper::Unwrap to obtain pointers
 - markdown() now checks that arguments have the correct types
 - implemented markdownSync()
@mildsunrise mildsunrise closed this May 4, 2012
@mildsunrise mildsunrise reopened this May 4, 2012
@benmills
Copy link
Owner

benmills commented May 4, 2012

First of all thank you for the thorough pull request! I've looked over most of the changes and they look great. The only small question I have is about the bumped version number (https://github.com/jmendeth/robotskirt/commit/57db8263742b92439a5aa981d46de85661ff07d7) it seems like if we want to make a major version bump we should change the version number to 1.0.0

What do you think about that?

@mildsunrise
Copy link
Collaborator Author

Yes, you're right.
In fact, the minor version was bumped many times (internally)
during the development, in addition to the major. That's why,
when I finally pushed package.json, the version was 1.2.2.

Ok, I'll reset it to 1.0.0.

@edufelipe
Copy link

If you gentlemen allow me to chime in: I think providing a compatibility toHtml(md) function, where an implicit renderer is created on every call, would be great.

The speed bump of reusing a renderer is relevant, but a point can be made for compatibility and simplicity of the API.

Just my 2 cents.

@benmills
Copy link
Owner

benmills commented May 7, 2012

I like @edufelipe suggestion. What do you think @jmendeth ?

@mildsunrise
Copy link
Collaborator Author

Yes, that would be great!
But it'll have to wait. Sorry, I'm very busy these days.

However, migrating older code to markdown is very easy.
Just replace rs.toHtml( with rs.markdown(new rs.HtmlRenderer(), and you're ready to go!

@edufelipe
Copy link

@jmendeth Yes, I do agree that it is very easy, but I rather have a clean API where an easy thing that needs to be done – before I accomplish what I set out to do – is automatically done for me.

@benmills
Copy link
Owner

benmills commented May 8, 2012

I agree with @edufelipe that a backwards compatiable API is very important. How would you both feel if I pulled in this code and make a second commit with the added code to support toHtml before I publish the new version to npm? That way @jmendeth won't have to spend extra time since he's already done so much.

@mildsunrise
Copy link
Collaborator Author

@edufelipe @benmills Ok, thanks.
One doubt: if you add a compatibility function,
then there's no need to bump the major, as it's still compatible?

@benmills
Copy link
Owner

benmills commented May 9, 2012

@jmendeth I always go back and forth on that. On one hand since we're not breaking the API we shouldn't do a major version bump. However we're adding quite a bit of new functionality and its pre-1.0 so I would think this seems like a good place to go 1.0.

I'm leaning towards going 1.0.

@mildsunrise
Copy link
Collaborator Author

@benmills OK, you lead the way! :)

benmills added a commit that referenced this pull request May 14, 2012
Improvements, extensions and some fixes
@benmills benmills merged commit 1414bff into benmills:master May 14, 2012
@benmills
Copy link
Owner

@jmendeth @edufelipe I was thinking more about the API before I publish robotskirt on npm. I'm a little worried about a method named markdown. Here is what I was thinking for an API:

var rs = require('robotskirt');

var renderer = new rs.HtmlRenderer();
var markdownString = "# hello world";

rs.renderMarkdown(renderer, markdownString, function (html) {
  //...
});

var html = rs.renderMarkdown(renderer, markdownString);

@mildsunrise
Copy link
Collaborator Author

Well, I just noticed the Sundown version of Robotskirt is very outdated!
The actual version allows creation, storing and reusing of Markdown objects
apart from Renderers. Each Markdown has a method render(...).

I'm working on it...
The finished version (2.0.0?) will also have support for custom renderers!
I've already planned the introspection system for it...

@benmills
Copy link
Owner

Yes I actually took a look at upgrading and just getting robotskirt working but it was not straight forward. I think I'm going to go with the API that looks like this going forward:

For raw markdown parsing use

rs.renderMarkdown
rs.renderMarkdownSync

For HTML or any other specific format we will provide a method that looks like to{Format}

toHtml
toHtmlSync

I'm busy this weekend but I'll try to get 1.0.0 out with your original pull request and the minor API change tagged and on npm early next week.

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.

3 participants