Skip to content

Miscellaneous improvements (will add more detail later) #26

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

Conversation

wlupton
Copy link
Contributor

@wlupton wlupton commented Jun 6, 2020

This PR contains several proposed changes for discussion. I've also created some companion PRs in other projects, all as part of setting up a new "markdown-only" sphinx project. For reference, here are the other PRs:

My goal was to avoid writing any ReST (apart from the necessary directives to pull in code documentation).

Here's a summary of the proposed changes:

  • new_section() - try to get the id from the heading and, if found, use it as the anchor text
  • start_new_section() - similarly try to get class from the heading and, if found, use it as the classes attribute
  • visit_a() - substantial rewrite based on CommonMarkParser.visit_link() logic
  • AutoStructify - add auto_toc_tree_maxdepth and auto_toc_tree_numbered options (and the corresponding logic); also changes to match the updated visit_a() and to explicitly ignore any link text when generating the ToC tree (otherwise the supplied text gets used, which you never want: you want the section names from the referenced files)

Here's what my main page looks like ( .display-none uses the above classes feature to enable CSS to hide the section; this seemed like the best compromise: you need a section to give to AutoStructify but it looks bad if actually displayed in the HTML):

# ONU simulator and test client

## Contents {.display-none}

<!-- toctree -->

* [](usage/introduction)
* [](usage/extensions)
* [](bin/index)
* [](obbaa_onusim/index)

## Indices and tables

* [Index](genindex)
* [Module Index](modindex)
* [Search Page](search)

Comments welcome. I'd be happy to make any suggested updates and/or split this into multiple PRs.

@clayrisser
Copy link
Owner

Thank you @wlupton wlupton

@clayrisser clayrisser merged commit e72768a into clayrisser:master Jun 7, 2020
@wlupton
Copy link
Contributor Author

wlupton commented Jun 8, 2020

Thanks @codejamninja. I feel that there's some more discussion to have here. For example:

``` automodule:: module-name
```
  • There are some open issues with regard to how links should be handled

  • There should probably be a more consistent policy re how attributes from underlying markdown objects are handled (I just handled id and class because I needed them)

  • I think that sphinx-markdown-parser needs its own AutoStructify documentation; it's now diverged from the recommonmark implementation

How best to handle? Should I create separate issues?

@wlupton
Copy link
Contributor Author

wlupton commented Jun 8, 2020

@codejamninja, please see the sphinx-doc/sphinx#7796 discussion. There's a suggestion that this crash could be fixed on the sphinx-markdown-parser side.

href = urllib.parse.urlunparse(r._replace(path = r.path[:-3] + ".html"))
except:
pass
url_check = urllib.parse.urlparse(href)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@wlupton the previous code added .html, is this redundant (does sphinx or other code add it automatically) or is the behaviour actually changed? Some of my projects depend on this behaviour.

Copy link
Contributor Author

@wlupton wlupton Jun 8, 2020

Choose a reason for hiding this comment

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

I just did a test with this input:

These should all work:

* See [this file](extensions.html) for more info
* See [this file](extensions.md) for more info
* See [this file](extensions) for more info

It's actually the first one that doesn't work (the other two do). It's not detected as a link at all. Is that expected?

But I guess this also needs testing where there's an explicit link to an HTML file outside the "sphinx domain"?

Update:

I'd missed that I had the following sphinx warning:

introduction.md:: WARNING: None:any reference target not found: extensions.html

So if this does need to work, it needs not to be an any reference. Using a file: URL should be sufficient to guarantee this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change it at all, what was wrong with the old version?

@wlupton
Copy link
Contributor Author

wlupton commented Jun 8, 2020

Note that I suggested (in Python-Markdown/markdown#977) that maybe we could tolerate having to write something like:

``` {rst="automodule:: module-name"}
```

or (with post-processing, e.g., -- -> :: ):

``` automodule--module-name
```

Thoughts?

@infinity0
Copy link
Collaborator

infinity0 commented Jun 8, 2020

@wlupton regarding your AutoStructify changes, the python-markdown parser was never designed to work with AutoStructify, which is based on recommonmark.

I wrote the python-markdown parser to mostly not need AutoStructify, since all the AutoStructify features are specific to recommonmark and basically are redundant with python-markdown. (I added special handling for mathjax here.) The only thing missing then is enable_eval_rst which personally I do not have a use for. I would just say in the documentation "markdown_parser.py does not work with AutoStructify" and leave it at that.

Copy link
Collaborator

@infinity0 infinity0 left a comment

Choose a reason for hiding this comment

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

Did you run the test suite against the current example page & output?

href = urllib.parse.urlunparse(r._replace(path = r.path[:-3] + ".html"))
except:
pass
url_check = urllib.parse.urlparse(href)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change it at all, what was wrong with the old version?

@infinity0
Copy link
Collaborator

@wlupton Your URL changes caused the test suite to fail (it generates a .md link instead of .html) so I've reverted for now.

@clayrisser
Copy link
Owner

I’m going to publish a release soon. If we could get all stable changes in by the end of this week that would be great.

@clayrisser
Copy link
Owner

#27

@wlupton
Copy link
Contributor Author

wlupton commented Jun 9, 2020

Thanks for the feedback, and please note that I never expected this to be merged without discussion (see my original comment)!

Without AutoStructify, how can you (in pure markdown) define the toc tree, or include automatically-generated code documentation?

Note that the motivation behind the link changes was to allow links to things like python objects and methods. For example, with these changes this works (to refer to the bin.onusim python module):

The [ONU simulation server](bin.onusim)...

I may pick this up again at a later date (probably split into multiple PRs).

@wlupton
Copy link
Contributor Author

wlupton commented Jun 9, 2020

@infinity0, the file transform.py appears to be a reimplementation of AutoStructify for CommonMark, so I don't quite understand what you mean by "was never designed to work with".

@infinity0
Copy link
Collaborator

Without AutoStructify, how can you (in pure markdown) define the toc tree, or include automatically-generated code documentation?

Sphinx does this automatically from the heading structure in the rST syntax tree.

@infinity0, the file transform.py appears to be a reimplementation of AutoStructify for CommonMark, so I don't quite understand what you mean by "was never designed to work with".

CommonMark and python-markdown are two entirely different parsers. AutoStructify was written for the former, not the latter. OTHO markdown-parser.py is based on the latter.

Let me give some more details: this project has a fairly messy history, and I just got into it a few months ago when I rewrote markdown-parser.py - the old version simply didn't work for me at all. When I wrote this current version, I had no intention of making it work for AutoStructify and didn't try out transform.py. The entire purpose-of-existence of AutoStructify is to post-process the rST syntax tree that recommonmark generates, and are not expected to work on the rST syntax tree that python-markdown generates.

OTOH, I intended this current version of markdown-parser.py to simply not need this type of post-processing. Whatever you need to do, you should be able to do it directly in markdown-parser.py, without AutoStructify post-processing. As I mentioned, apart from enable_eval_rst its features are all largely redundant compared to this current version of markdown-parser.py. python-markdown has its own extensions, pymdownx so I spent most of my effort supporting these - though the support isn't perfect since you have to ad-hoc translate markdown AST to a rST AST.

Note that the motivation behind the link changes was to allow links to things like python objects and methods [..]

OK great, please do file PRs! However in my personal opinion rST is superior to markdown in every way, I would suggest it's simply easier for everyone involved to just use sphinx's existing support for that sort of autdoc, based on rST syntax. I only contributed to this project because some other constraints require me to support both markdown and rST in the same sphinx project, and the existing other sphinx-md parsers didn't fit my requirements e.g. mathjax support.

@infinity0
Copy link
Collaborator

reimplementation of AutoStructify

It's not a reimplementation, recommonmark is commonmark. I believe our transform.py is more-or-less copied from one of the older versions of transform.py

@FlorianLudwig
Copy link
Contributor

Note that the motivation behind the link changes was to allow links to things like python objects and methods [..]

@wlupton I would like to see that :) Is your work public somewhere?

@wlupton
Copy link
Contributor Author

wlupton commented Sep 27, 2020

@FlorianLudwig, you can see my changes in https://github.com/BroadbandForum/sphinx-markdown-parser/tree/feature/misc-improvements. This PR was merged but a later commit (9fdf309) reverted the changes to the visit_a() function.

As part of getting all this working, I created PRs in several repos, most of which were not well-received and were not merged. I haven't (as yet) followed this up but will at some point rebase all my stuff against all the latest versions, take feedback into consideration, and have another go.

Let me know if you'd like a full list of the affected repos and PRs.

@clayrisser
Copy link
Owner

once everything is ready let me know and I will accept the changes and release an update

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.

4 participants