Skip to content
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

reST support #56

Closed
wants to merge 6 commits into from
Closed

reST support #56

wants to merge 6 commits into from

Conversation

ternstor
Copy link

@ternstor ternstor commented Apr 6, 2012

I added basic reST support to pycco, using creole's rest2html(). What are your thoughts on this?

Regards,
Thomas

@treyhunner
Copy link
Collaborator

I definitely like the idea. reStructuredText is probably more commonly used in Python code than markdown.

I'll test this soon. Feel free to recommend any documentation heavy codes base using reST to test this against.

Thanks for the pull request.

@kennethreitz
Copy link

YES I WANT THIS

@treyhunner
Copy link
Collaborator

I ran this new feature against Sphinx and Requests repositories.

Some issues I've found so far:

  • ~~~ headers are not parsed as headers (=== and --- work fine)
  • None of the field lists and nor any of the .. references that I found were handled specially
  • Double colons used to denote literal blocks are ignored (example)

These may be due to limitations in python-creole.

Unfortunately I haven't found any code in the wild that this generates reasonable HTML yet. We may need to create a sample file to test out this feature.

@ternstor
Copy link
Author

ternstor commented Apr 6, 2012

Hi Trey, thanks for the feedback,

I found that SYNTAX_FUNC was not being changed to reST, as it was being created locally inside main(). See the last commit for more details. You may want to try this feature again.

I tried running the feature against the Request repo, and docutils errors and warnings are being printed by rest2html() into the html. I initially just ran it against a well formed reST'ed code, so I wasn't aware of this. The errors were created because :class: is not a native reST keyword and is defined by sphinx. I agree that running pycco should try to just work for code in the wild, so I'll figure out how to deal with sphinx and disable warning/error output into the html.

I'll let you know next week if I've figured out this.

@treyhunner
Copy link
Collaborator

Hey Thomas,

You're right. I actually hadn't been running it through rest2html properly.

I now see those warnings and errors you mention when running against requests repository. The top level __init__.py looks pretty bad and packages/oreos/core.py completely breaks the process, stopping further parsing.

reST support does seems to work well for some files (auth.py, async.py, and defaults.py in requests), but there are definitely some issues that need to be worked out.

Let me know if you make more progress on this. I'll be testing this feature out on my own code in the meantime.

@adambard
Copy link

adambard commented May 9, 2012

Why not use docutils, the origin of reST? It's almost as easy as using docutils.core.publish_parts.

I'm using this on some internal stuff right now. Admittedly I had to do a pretty ugly hack to get it to work on sphinx, but it does work.

diff --git a/pycco/main.py b/pycco/main.py
index 105a7cb..5c9c7e3 100644
--- a/pycco/main.py
+++ b/pycco/main.py
@@ -215,10 +215,25 @@ def highlight(source, sections, preserve_paths=True, outdir=None):
             docs_text = unicode(section["docs_text"])
         except UnicodeError:
             docs_text = unicode(section["docs_text"].decode('utf-8'))
-        section["docs_html"] = markdown(preprocess(docs_text,
-                                                   i,
-                                                   preserve_paths=preserve_path
-                                                   outdir=outdir))
+
+
+        body = preprocess(docs_text, i, preserve_paths=preserve_paths, outdir=outdir)
+
+        # Prepare for reST.
+        lines = body.split("\n")
+        body = re.sub("\t", "    ", body) # Not that tab-users deserve consideration
+
+        # docutils hates it when things are indented. Bring things back to the base
+        min_indent = 0
+        indent_lens = map(len, [re.match(' *', l).group(0) for l in lines if l])
+        if indent_lens:
+            min_indent = min(indent_lens)
+        body = re.sub("^" + ' ' * min_indent, '', body, flags=re.M)
+
+        reST = lambda s: docutils.core.publish_parts(s, writer_name='html')['body']
+        section["docs_html"] = reST(body)
         section["num"] = i

 # === HTML Code generation ===
@@ -269,6 +284,7 @@ import re
 import sys
 import time
 from markdown import markdown
+import docutils.core
 from os import path
 from pygments import lexers, formatters

@treyhunner
Copy link
Collaborator

@adambard Using your code unfortunately doesn't fix the issue I have when running this on the requests code. I believe rest2html relies on docutils.

When running the following on the requests code:

pycco -s reST -p requests/packages/oreos/monkeys.py

I receive:

  File "/usr/local/lib/python2.6/dist-packages/docutils-0.7-py2.6.egg/docutils/utils.py", line 203, in system_message
    raise SystemMessage(msg, level)
docutils.utils.SystemMessage: <string>:1: (SEVERE/4) Incomplete section title.

##########################################################
Backwards Compatibility:  Don't break any existing code!

@adambard
Copy link

oreos/monkeys.py looks to be commented in markdown. Docutils tends to be very picky about its input, so maybe it's not a good fit here. It just jumped into my head when I saw reST mentioned.

Then again, perhaps it could be worked to use docutils reST first, and markdown as a fallback, since markdown will do a reasonable job on reST (if not vice-versa). That really starts to pile the requirements on though.

@kennethreitz
Copy link

oreos should be removed in the next release, fwiw :)

@willkg
Copy link

willkg commented Jul 2, 2012

I can't tell where this is at and whether it's blocking on anything. Is there a "final-ish" version of reST support? If so, where is it? Does it just need more testing or are there more bugs to be fixed?

@treyhunner
Copy link
Collaborator

I think this needs more testing and probably at a minimum some bug fixes for exception handling (when invalid reST is found).

@keir
Copy link

keir commented May 21, 2013

Any movement on this? I'd love to see reST in Pycco.

@treyhunner
Copy link
Collaborator

I have issues with merging this without confidence that:

  • existing functionality will be maintained (old stuff won't break)
  • the feature actually works reasonably well

As I see it, the best way to ensure both of those would be to add tests to Pycco (#54). Pycco has no test suite. No tests means the users are the testers. I use Pycco very rarely now so I'm not comfortable considering myself a tester nor giving new nontrivial features a stamp of approval.

I welcome any progress toward adding tests to Pycco. Also if anyone that uses Pycco more often than I do wants to help maintain, please say so.

@gamesbook
Copy link

Would also like to see it... can help with running the tests.

@wmayner
Copy link

wmayner commented Mar 12, 2014

+1, this would be awesome.

@carlos-jenkins
Copy link

Pycco has no test suite. No tests means the users are the testers

@treyhunner, ok, but if you never merge, users will never try it. If you break something, users will complain, if they don't complain, everything is alright.

+1 for this feature.

@treyhunner
Copy link
Collaborator

@carlos-jenkins valid point. I had some issues with this the last time I try it. If a couple people are willing to test it out and vouch for its merge-worthiness I'll go ahead and merge it.

Without a test suite, dog fooding is necessary for testing. I haven't used pycco in a long time so I haven't been able to test these features myself.

I think this project is in need of more active users with push access. Anyone volunteer to help test and merge/close open pull requests?

@ternstor ternstor closed this Sep 6, 2017
@stefano-bragaglia
Copy link

+1 for this feature too!

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.

10 participants