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

Alternative way to make anchor_link_text configurable #522

Merged
merged 1 commit into from May 15, 2017

Conversation

Projects
None yet
2 participants
@takluyver
Copy link
Member

takluyver commented Jan 29, 2017

Closes gh-412, alternative to gh-520

@takluyver takluyver referenced this pull request Jan 29, 2017

Closed

Configurable filter #520

def default_filters(self):
for pair in super(HTMLExporter, self).default_filters():
yield pair
yield ('markdown2html', self.markdown2html)

This comment has been minimized.

@takluyver

takluyver Jan 29, 2017

Author Member

@michaelpacer I think this is the main difference from #508: rather than passing the new option through the template to the existing filter, I've replaced the markdown2html filter with one that follows the config option.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jan 31, 2017

One of the features that I don't like about this is that it puts a filter inside the html exporter. This would mean for someone who wanted to grab the filter from nbconvert, they would need to know to go to the exporter to find the filter as opposed to the nbconvert.filters. But if someone were to try to find a filter, we generally would point them to the filters module. Then, someone trying to use markdown2html as it exists in the filters module may suddenly have different behaviour than what they expected if they thought they were using the filter used by the html exporter.

I think we do have filters that are defined within exporters elsewhere, but I think it somewhat breaks nbconvert's architectural promises.

In addition to these user facing concerns it introduces some software dev issues. This PR overwrites a function inside this exporter that is only used (in first party code) by this exporter. That means that now we have to maintain two different pieces of code that now have different functionality, but the same name and are used to (ostensibly) accomplish the same things. To the extent that we think that markdown2html is used by 3rd party developers, I agree that it's important to keep that there for API stability. However, to the same extent the previous problem of people not being able to access the actual functionality we "intend" when we use our html exporter's version of markdown2html.

I understand why this is the kind approach that "needs" to be pursued in order to be able to apply the configuration here (given that filters are functions and therefore can't easily inherit configuration values from their parents). But that's exactly why making filters themselves configurable in the vein of #520 (i.e., without breaking any APIs) seems like (to me at least) the way to go.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jan 31, 2017

I don't think there's anything wrong with an exporter redefining a generic filter to do something more specific (in this case, using a config option on the exporter).

we have to maintain two different pieces of code that now have different functionality

Not really, to my mind. The new one is a trivial wrapper around the old one. Having configurable filters seems like more maintenance burden.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented May 15, 2017

I'm just going to merge this and close the other.

@mpacer mpacer merged commit bb12c8b into jupyter:master May 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mpacer mpacer added this to the 5.2 milestone May 24, 2017

@mpacer mpacer added to_changelog and removed to_changelog labels May 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment