search

Python-Markdown / markdown

A Python implementation of John Gruber’s Markdown with Extension support.
https://python-markdown.github.io/
BSD 3-Clause "New" or "Revised" License
3.74k stars 858 forks source link

Backwards-incompatible change to TableProcessor constructor in 3.4 #1278

Closed tsibley closed 2 years ago

tsibley commented 2 years ago

The markdown.extensions.tables.TableProcessor constructor changed signature from parser to parser, config in 659a43659c6012df8d8ceb4a3681d2ddb1cb7540, first released with 3.4. This doesn't appear to be noted in the 3.4 release notes.

I don't know if TableProcessor is considered part of the public API or not, but I don't see any indications that it isn't. Opening a new issue per https://github.com/Python-Markdown/markdown/issues/1277 since this change doesn't appear to be the removal of a previously-deprecated calling signature. If this isn't a public API, then this can be closed. If it is a public API, could a default at the TableProcessor (not just TableExtension) level be added?

This change broke the sphinx_markdown_tables package; see https://github.com/ryanfox/sphinx-markdown-tables/issues/36.

facelessuser commented 2 years ago

As far as I am aware, what they are doing here, is not what the plugin system was designed for. Now, I'm not saying you "can't" do that, but this isn't really a public API as much as an implementation detail. There are times I have been guilty of digging deeper into the machinery, but I do so, acknowledging, "Here be dragons". I believe that is also the case here.

As this is the first time this specific plugin has exposed settings, it makes sense that now it is accepting the config so that it can reference them from now on.

If there is any question about what is considered the 'public API', it is laid out in the documentation.

waylan commented 2 years ago

I agree with everything @facelessuser said. You are subclassing a class defined in an extension, which is not really a public API.

What could we have done differently? Maybe this:

processor = TableProcessor(md.parser)
processor.config = self.getConfigs()

That would avoid changing the class signature, but it would still require the config to be set for an instance of the class to run. In other words, you would still need to modify your subclass anyway.

In the end, we documented a backward incompatable change which explained that the behavior of the extension changed such that it now accepts a config option. That should be enough to prompt anyone subclassing our extension to go spelunking in the code to see if our changes may break anything.

tsibley commented 2 years ago

Ok, thanks for the responses. I suspected as much re: this not being a public-enough API but wanted to check. I'll close this issue.

@waylan Because you said "you", do note that it's not my subclass or package. I'm just a user of that sphinx-markdown-tables package (which is fairly widely used).

waylan commented 2 years ago

@waylan Because you said "you", do note that it's not my subclass or package. I'm just a user of that sphinx-markdown-tables package

Thanks for the clarification. I guess I made an assumption there. Sorry about that. I should have said "the developers of the extension" instead.