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

Adding extensible, type-specific serialization for code blocks. #79

Merged
merged 1 commit into from Jun 4, 2013

Conversation

jbunting
Copy link
Contributor

My goal was to allow special handling of specific code blocks. I think this is a pretty succinct solution. I toyed with the idea of using the ServiceLoader to automatically load any VerbatimSerializers registered on the classpath, but decided to go the more manual route. Let me know if you want me to add some more automated extension loading -- or if you have any other questions or concerns about it.

@sirthias
Copy link
Owner

Interesting, thanks Jared.
Could you add some tests as well?

@jbunting
Copy link
Contributor Author

A default serializer makes sense - I hadn't really settled on the best way to handle that. What I'm leaning towards right now is just a second field, "defaultSerializer" with a setter. Thoughts?

Sorry about the whitespace change -- I'll rework this without that and with some tests.

@sirthias
Copy link
Owner

I don't think we need a second field. Rather please make the verbatimSerializers member final and allow for passing in the map in the constructor. If the user doesn't explicitly pass in a list we'll only have one map element: the default serializer. Otherwise we add the default serializer to the given map if it doesn't contain a serializer for the default key already.

@jbunting
Copy link
Contributor Author

Sounds good. Any preferences on what the default key is?

@sirthias
Copy link
Owner

How about you add a static DEFAULT String member to the VerbatimSerializer interface?
Value: "DEFAULT"

@jbunting
Copy link
Contributor Author

Got that added. Now I'm working on the test - I realized that when i was running this myself, I was overriding the markdownToHtml(char[], LinkRenderer) method to instantiate the ToHtmlSerializer myself. This doesn't seem like the best pattern, but I'm not really sure which way to go here. Should I overload markdownToHtml with markdownToHtml(String, LinkRenderer, Map<String, VerbatimSerializer>) and markdownToHtml(char[], LinkRenderer, Map<String, VerbatimSerializer>) or something else? My first thought was to pass the VerbatimSerializer map in the PegDownProcessor constructor, but that doesn't really seem inline with how you're currently passing in the LinkRenderer.

@sirthias
Copy link
Owner

Should I overload markdownToHtml with markdownToHtml(String, LinkRenderer, Map<String, VerbatimSerializer>) and markdownToHtml(char[], LinkRenderer, Map<String, VerbatimSerializer>)?

Yes, I think that is the best of the mediocre solutions that Java let's us do here. If at some point another custom rendering component should be added we'll have to think about another design, but for the time being this will be ok.
Thanks!

@jbunting
Copy link
Contributor Author

I think that's all there now. There is one unrelated change - in the casing of the file name for one of the tests. I can pull that out if you'd like, but I needed it to get the tests to pass.

Let me know if there are any issues with style in the tests - sbt and specs2 are new to me (and my Scala experience in general is limited).

sirthias added a commit that referenced this pull request Jun 4, 2013
Adding extensible, type-specific serialization for code blocks.
@sirthias sirthias merged commit 04305cf into sirthias:master Jun 4, 2013
@sirthias
Copy link
Owner

sirthias commented Jun 4, 2013

Thanks, Jared!

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

2 participants