Skip to content

Automatic support for ipython3 code blocks #79

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

Merged
merged 15 commits into from
Dec 14, 2021

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Dec 6, 2021

My main goal is to use this extension together with https://nbsphinx.readthedocs.io/ and to make it as easy as possible for others to do so as well.

#75 already helped to avoid having to specify custom CSS classes and this PR avoids having to specify codeautolink_custom_blocks.

For reference, here are some changes on the nbsphinx side:
spatialaudio/nbsphinx#613
spatialaudio/nbsphinx#614

With those changes in place, the only thing that users have to do is to add 'sphinx_codeautolink' to extensions. And that's exactly the amount they should have to do (i.e. I don't want to enable it automatically when loading nbsphinx).


  • Tests written and passed
  • Documentation and changelog entry written, docs build passed
  • All tox checks passed

@felix-hilden
Copy link
Owner

Thanks for the submission!

The changes look alright and the use case is reasonable. You've even made the IPython import dynamic which is nice. Still, I'm a bit hesitant because I don't really like the idea of depending on many external packages, or starting to build a library of parser functions. I'm not entirely sure why though, so if you have any insight into how these kinds of situations have been handled in other extensions I'd greatly appreciate it. For example, the reason I included matplotlib's plot_directive in #45 was because it already worked out of the box. And Oriol actually implemented a virtually identical parser as a result of the custom block addition, which was developed for this IPython case specifically.

Maybe if we go this route of starting to add additional parsers, we could make a submodule out of the parsers. And certainly IPython is one of the more popular Python syntax supersets.

I'll think about it!

@mgeier
Copy link
Contributor Author

mgeier commented Dec 7, 2021

Still, I'm a bit hesitant because I don't really like the idea of depending on many external packages, ...

Understandable. I still think it's worth it.

... or starting to build a library of parser functions.

Why not?

The list of the most common parsers will be very short, you don't have to implement any exotic formats.

I'm not entirely sure why though, so if you have any insight into how these kinds of situations have been handled in other extensions I'd greatly appreciate it.

In my own extensions I try to make their usage as simple as possible.

For many people, using Sphinx is on its own already very complicated, and every extension that has cryptic custom settings makes the situation even more complicated. If it's too complicated, many users will simply give up, which would be a pity.

I see no reason why adding 'sphinx_codeautolink' to extensions shouldn't be sufficient. This is exactly how I would expect it to work.

If you don't implement it here, I will implement it in nbsphinx. However, there it would be much more complicated and hacky (especially when trying to avoid the caching problem described in #76), so I'd really prefer if it could be implemented here.

Maybe if we go this route of starting to add additional parsers, we could make a submodule out of the parsers.

I'm not sure if that's worth it for a few lines of code.

And do you even expect requests for any other Python dialects?

Cython? SageMath?

I think the list is very short.

@felix-hilden
Copy link
Owner

That's fair, I think I've turned around. Honestly I don't quite know about more esoteric Python dialects, but they are just that: esoteric. As you said, I wouldn't honestly expect much more than IPython.

I'm thinking here's what we should do:

  • add an install extra for IPython
  • add checks to only parse IPython blocks when the import would succeed
  • document all of this

This would result in the following: users would install sphinx-codeautolink[ipython] if they don't already have it installed, then simply activating the extension enables our version of IPython parsing. How does that sound?

@mgeier
Copy link
Contributor Author

mgeier commented Dec 8, 2021

Sounds good!

I'm just not sure how to best make the import check. I've created e354b6c as a possible implementation, but maybe you have a better idea?

@mgeier
Copy link
Contributor Author

mgeier commented Dec 8, 2021

BTW, what's the 'py' type? It doesn't seem to be handled as highlight-pyCSS class, but it seems to be parsed?

@felix-hilden
Copy link
Owner

Thanks for the update! I'll probably edit this branch to finish the work as I laid out above.

The py lexer is simply an alias of python. Both are valid language parameters in code blocks!

@mgeier
Copy link
Contributor Author

mgeier commented Dec 8, 2021

I'll probably edit this branch to finish the work as I laid out above.

I can finish it, no problem. I just wanted to wait for your comments.

The py lexer is simply an alias of python. Both are valid language parameters in code blocks!

Interesting, I didn't know that!

But then I've found a bug: The CSS class highlight-py is not correctly handled.

I can fix this while I'm at it ...

Copy link
Owner

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, reviews are fine to me 😄 Here are some initial ones.

Big picture items:

  • Docs
  • Tests with IPython

Copy link
Owner

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting there!

We probably ought to add at least a mention of this, if not a full example to our docs under "IPython blocks". Perhaps between doctest and third-party blocks since it's more third-party than doctest but less than matplotlib 😄

@mgeier
Copy link
Contributor Author

mgeier commented Dec 10, 2021

We probably ought to add at least a mention of this, if not a full example to our docs under "IPython blocks".

I'm not sure if we should mention this.

The problem is that I don't know a realistic example where I would want to use the ipython3 language in an explicit reST code block.
Do you know one?

For me, this is mostly relevant in combination with other extensions (like e.g. nbsphinx), and users don't really need documentation about this.

Of course we could add a Jupyter notebook to the docs, but this seems a bit much.

@felix-hilden
Copy link
Owner

I think either is fine, although a short IPython syntax code block is totally fine. Users are probably able to extrapolate the use case to their notebooks. But we definitely will add some example about this 😄 Just the same toy code will do, preferably with some of those magic lines.

@mgeier
Copy link
Contributor Author

mgeier commented Dec 13, 2021

I've added a small section about IPython to the docs: dfa6220.

Copy link
Owner

@felix-hilden felix-hilden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think this is ready to go! Thanks for being patient with me and quick with the updates 👍

@felix-hilden felix-hilden merged commit b84b24a into felix-hilden:master Dec 14, 2021
@mgeier mgeier deleted the clean-ipython3 branch December 14, 2021 20:24
@mgeier
Copy link
Contributor Author

mgeier commented Dec 14, 2021

No problem, I like a thorough review!

Thanks for merging!

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

Successfully merging this pull request may close these issues.

2 participants