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

Backwards compatibility broken on #markdown2html() #1007

Closed
paos opened this issue May 1, 2019 · 10 comments
Closed

Backwards compatibility broken on #markdown2html() #1007

paos opened this issue May 1, 2019 · 10 comments
Labels

Comments

@paos
Copy link

paos commented May 1, 2019

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?

@MSeal MSeal added good first issue great for new contributors help wanted labels May 6, 2019
@erinspace
Copy link

Looking into this at the PyCon sprints!

@ivanov
Copy link
Member

ivanov commented May 6, 2019

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
Copy link
Collaborator

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

@maartenbreddels
Copy link
Collaborator

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

Looking at

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
Copy link

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

@ivanov
Copy link
Member

ivanov commented May 6, 2019

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
Copy link
Collaborator

@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
Copy link
Contributor

MSeal commented May 6, 2019

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
Copy link
Collaborator

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
Copy link
Author

paos commented May 7, 2019

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants