Backwards compatibility broken on #markdown2html()

[paos]

Compatibility with fast.ai's librarys doc() method is broken. Using it now creates an error due to the latest update in markdown2html.

Commit 654d6a3 changes the method signature:

- def markdown2html(self, source):
+ def markdown2html(self, context, source):

fast.ai's librarys doc() method calls output = HTMLExporter().markdown2html(md)

Is it possible to make the change backwards compatible instead of introducing a mandatory extra argument in front of the old one?

[erinspace]

Looking into this at the PyCon sprints!

[ivanov]

The plan is to revert this backwards incompatible change and have the next release include context as the last argument, with a default value of None that will create an empty dictionary in the method body. That way we keep the older code from when this method only took one parameter of source working without making any changes.

tl;dr - flip it and reverse it!

[maartenbreddels]

I'm not sure that works, since this method is called by jinja2, please check if the argument order matters.

[maartenbreddels]

Reverting this would break voila: https://github.com/QuantStack/voila/blob/807f635d56fc741d1029dbe31b1a51841a8fb996/voila/html.py#L22

Looking at

• https://github.com/pallets/jinja/blob/194c9d4135dba0fdd9357f9ce8b537c14c99aa36/jinja2/filters.py#L29
• https://github.com/pallets/jinja/blob/fb7e12cce67b9849899f934e697f7e2a91d604c2/jinja2/environment.py#L440

It seems the order does matter. I don't think markdown2html should be a public API, it's a filter used for jinja2.
However, a possible solution (although ugly) would be to have the second argument default to None, and check if is not instanceof(context, jinja2.nodes.EvalContext), and if so, it probably means the first argument was the source.

[erinspace]

Seems like there's a bit more discussion that should take place, going to drop this and focus on another error for PyCon :)

[ivanov]

Thanks for chiming in, @maartenbreddels. As I told @erinspace when we started looking into it, this is a tricky issue because no matter what we do, we will break someone code. The question is, which code should we break - those who have been extending HTMLExporter and overwriting the markdown2html or those using the new markdown2html parameter order. @MSeal and I had the thought process that markdown2html has been part of the public API for a long time, since it's a method on HTMLExporter, and this API changed in a backwards incompatible way only in the release a month ago.

Is there another way to get around the unfortunate parameter ordering that contextfilter imposes, so we could achieve the def markdown2html(self, source, [context]) parameter ordering?

[maartenbreddels]

@MSeal and I had the thought process that markdown2html has been part of the public API for a long time

Given that it is not documented, e.g. not part of the generated documentation, I assumed it was not a public API, which does not mean people use it, and it has become it.

Is there another way to get around the unfortunate parameter ordering that contextfilter imposes

A good lesson for API design, this choice of adding parameters is very unfortunate.

why not do *args, test for the length and based on that make the right decision, it's ugly, but it doesn't break anything, and gives no hints about the argument.

[MSeal]

Hmmm yeah I think you're right that this is the contract for jinja and it's a jinja / internal function call. I believe *args might cause problems for jinja?

I'll open an issue on fastai for them to change the pattern there.

[maartenbreddels]

Looking at the jinja code it will work, but I'm not sure we want to depend on that.

We could have a method markdown2html, which is not used for the filter, and add a method _markdown2html_filter that is used for the jinja filter. That, however, would break for subclasses that override markdown2html, since they will not be called (voila for instance).

Note that subclasses are free to override markdown2html and drop the decorator.

[paos]

fastai resolved this with a version check fastai/fastai@21faa5d that forks the method invocation to match markdown2html's signature before and after the update, so the issue is resolved for the part of fastai users. Thanks a lot for the help!