miyuchina / mistletoe

A fast, extensible and spec-compliant Markdown parser in pure Python.
MIT License
811 stars 113 forks source link

fix: Make filtering children by class in traverse() actually work #157

Closed asb closed 2 years ago

asb commented 2 years ago

The traverse() utility function was previously written to filter children by class using issubclass(child, klass):. But the first argument to issubclass() must be a class so this will always raise an exception when trying to use the functionality. This patch corrects the call to isinstance(child, klass) and adds a test.

asb commented 2 years ago

@pbodnar: thanks for such a rapid review, I've made the suggested changes.

It's an unfortunate bug, but the traverse utility does seem pretty handy to have in the codebase. It's nice to have a helper to make it easier to do information extraction or even light modifications at the AST level, as an alternative/companion to customising the renderer.

pbodnar commented 2 years ago

@asb, thanks for the quick fix. :)

asb commented 2 years ago

I'm now wondering what the behaviour should be when specifying a class to filter on and also setting include_source=True. As it is, the 'source' node will be yielded even if it doesn't match the filter. Perhaps it doesn't matter much either way, but this seems a little surprising to me. It also doesn't seem too important one way or another though.

Given filtering by class didn't work previously, there's an opportunity to tweak this without breaking anyone's code.

pbodnar commented 2 years ago

@asb, good point. I can imagine that most people would expect, like you, that filtering by class will always win over include_source=True. In that case, to follow the principle of least surprise, we should probably change the current behavior. Are you willing to do the change yourself (change impl. + doc + unit test)?

asb commented 2 years ago

Sure thing - see https://github.com/miyuchina/mistletoe/pull/158