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

Configurable Markdown extensions #74

Merged
merged 6 commits into from
Aug 12, 2014
Merged

Configurable Markdown extensions #74

merged 6 commits into from
Aug 12, 2014

Conversation

sampsyo
Copy link
Contributor

@sampsyo sampsyo commented Apr 10, 2014

This lets the YAML configuration specify a list of Python Markdown extensions
to enable (on top of the default set). This is especially handy, for example,
when you want nice typography via the SmartyPants extension.

@SvenDowideit
Copy link

yay +1, I was going to do this too - needs documentation tho :) And I'll bet @tomchristie will talk about tests.

@sampsyo
Copy link
Contributor Author

sampsyo commented Apr 10, 2014

Good call. I added a section to the docs and fixed a bug that would have been caught by trivial testing. 😳

I'm not sure what the best testing strategy is here, but I'll look into it if I get a moment.

@sampsyo
Copy link
Contributor Author

sampsyo commented Apr 15, 2014

Test is in place now too. Let me know if there's anything else I can do to help.

@sampsyo
Copy link
Contributor Author

sampsyo commented May 16, 2014

Is there anything I can do to help get this ready to merge? I'd love to avoid diverging my fork too much from mainline.

"""

# Prepend a table of contents marker for the TOC extension
markdown_source = toc.pre_process(markdown_source)

# Generate the HTML from the markdown source
md = markdown.Markdown(extensions=['meta', 'toc', 'tables', 'fenced_code'])
md = markdown.Markdown(
extensions=['meta', 'toc', 'tables', 'fenced_code'] + list(extensions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might we want to use sets here? What happens when extensions includes 'toc'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I added a test (see below) and the Markdown module just seems to ignore duplicates. We could use a set here for future-robustness, but it is not currently necessary. Have a preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave it as-is. Just wrapping this in a set() wouldn't guard against multiple invocations if one of them had parameters, and sets sort their contents:

>>> range(5,1,-1)
[5, 4, 3, 2]
>>> set(range(5,1,-1))
set([2, 3, 4, 5])

Of course, mine isn't the opinion that counts. I really hope this and #97 get merged whenever @tomchristie next has a chance to hack on this. :)

If I have a chance later this week, I may send a pull request against your branch to flesh out the docs re: passing parameters to Markdown extensions.

@edbrannin
Copy link
Contributor

@sampsyo: I think this merge conflict is just that 2 different things were appended to the same documentation file. It should be really easy to resolve. :)

@tomchristie
Copy link
Member

Apologies to all for being so shitty at getting time on this project lately - it will come - I've just got a bunch of other stuff prioritised right now.

@edbrannin
Copy link
Contributor

I usually don't have this kind of time for side-projects either. Thanks for doing this at all! I'll just keep using my fork with this & my Windows pull-requests until you're able to examine & integrate-or-iterate them.

@sampsyo
Copy link
Contributor Author

sampsyo commented Jun 22, 2014

I concur with @edbrannin—thanks for being involved at all! Open-source maintainers know that Actual Life takes priority.

Thanks for the tip; I've rebased the PR on master.

@sampsyo sampsyo mentioned this pull request Jul 13, 2014
html_base, _, _ = build.convert_markdown(md_input)
self.assertEqual(html_base.strip(), expected_without_smartstrong)

# Check that the plugin is not active when not requested.
Copy link
Member

Choose a reason for hiding this comment

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

copy paste error with this line?

This lets the YAML configuration specify a list of Python Markdown extensions
to enable (on top of the default set). This is especially handy, for example,
when you want nice typography via the SmartyPants extension.
This lets the value be a tuple as well as a list. (Never really understood why
Python won't concatenate lists and tuples.)
I didn't use the SmartyPants extension here because the version of
python-markdown pinned here (2.3.1) doesn't include the extension. 2.4 does;
maybe the project should consider updating this dependency.
@sampsyo
Copy link
Contributor Author

sampsyo commented Aug 8, 2014

Thanks for pointing this out, @d0ugal. Fixed the comment in that test.

I also rebased again for a clean merge. Anything else I can do to help?

@d0ugal
Copy link
Member

d0ugal commented Aug 10, 2014

I'm going to give this a manual test over the next few days and get back to you.

@d0ugal
Copy link
Member

d0ugal commented Aug 11, 2014

Should the default be:

mardown_extensions=['meta', 'toc', 'tables', 'fenced_code']

and then we don't harcode any in? I'm just wondering if there is ever a case where somebody would want to remove one that is included by default? Could it clash with another plugin for example?

Otherwise I've tested this and it works well, I think it's ready to merge.

@sampsyo
Copy link
Contributor Author

sampsyo commented Aug 11, 2014

Hmm, this does seem like a good idea to me—it might be nice, for example, if you're porting Markdown from another system and want to match its behavior as closely as possible. Anyone else have an opinion either way? I'd be happy to make the change.

@d0ugal
Copy link
Member

d0ugal commented Aug 12, 2014

I'll ping @tomchristie to weigh in when he gets a moment. I'm mostly ambivalent, the main concern I see is that changing this later could be hard, as people may need to update configs.

@tomchristie
Copy link
Member

Pretty sure everything would break horribly without toc, not sure about meta. The other two I think are purely formatting style.

@d0ugal
Copy link
Member

d0ugal commented Aug 12, 2014

Having those two hard coded and the others optional might get confusing.

So, I think its best to just keep this simple and stay with the current approach, we can figure it out later if this causes an issue. If we all agree, I'll hit the merge button.

@tomchristie
Copy link
Member

Fine by me. I'm still not super-excited about enabling programmatic extensions, but happy to defer to others in this case. It does also tie us in strongly to this particular markdown implementation, but then again that isn't likely to change anytime soon, so probably not really a biggie. Side note that I'd like us to strongly avoid any further programmatic API beyond this.

@d0ugal
Copy link
Member

d0ugal commented Aug 12, 2014

Agreed - I think the markdown extensions are so common it's worthwhile in this case, I've used them a bunch in the past. I can't see anything else being such an obvious requirement.

d0ugal added a commit that referenced this pull request Aug 12, 2014
Configurable Markdown extensions
@d0ugal d0ugal merged commit db0644d into mkdocs:master Aug 12, 2014
@sampsyo
Copy link
Contributor Author

sampsyo commented Aug 12, 2014

✨ Thanks, everybody!

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

5 participants