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
adding complete code to the annotated html-file #2858
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with the feature in general. However, the C code can be very large and the HTML-ification time consuming, so yes, I think this should use a new directive (similar to annotate=True
). Not sure what a good name would be…
Cython/Compiler/Annotate.py
Outdated
@@ -228,7 +232,7 @@ def annotate(match): | |||
return u"<span class='%s'>%s</span>" % ( | |||
group_name, match.group(group_name)) | |||
|
|||
lines = self._htmlify_code(cython_code).splitlines() | |||
lines = self._htmlify_code(cython_code, True).splitlines() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing things like True
and False
into a function without further context screams for a named keyword argument to make it clear what the value means. However, in this case, it should actually use a language name: "cython"
vs. "c"
makes it even clearer what is intended.
c244f50
to
1007471
Compare
@scoder Maybe it would make sense to introduce different levels of "annotate":
See for example the current state of this PR. Or is this too hacky? |
1007471
to
0054e74
Compare
0054e74
to
63c1008
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with adding this, but would be happy about a more obvious interface.
A test is also missing, see TestIPythonMagic.py
.
Cython/Build/IpythonMagic.py
Outdated
'-a', '--annotate', action='store_true', default=False, | ||
help="Produce a colorized HTML version of the source." | ||
'-a', '--annotate', nargs='?', const=1, type=int, | ||
help="Produce a colorized HTML version of the source. Use --annotate=2 to include complete generated C/C++-code." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to at least restrict the option values to 1
or 2
and nothing else, but actually, this option still doesn't feel right to me. When I see --annotate=2
, I wouldn't know what that does.
63c1008
to
4caa56a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a regex check in tests/run/annotate_html.pyx
that this is disabled by default.
I think it's then also worth adding a new test module with a directive # cython: annotate=fullc
at the top that tests this new feature.
Oh, and there is probably also a way to test the interaction with IPython in |
c66122c
to
a2ab67f
Compare
Done.
Done
I'm not sure I understand. I have already added test cases in TestIpythonMagic.py (4caa56a) but this is not what you mean?
IIRC for that to work "annotate" must be a compiler-directive but right now it is an option. |
Sorry, my fault. They are ok. I somehow missed them in the last review, probably just looking at selected changes.
It's … both. And it would be weird if this feature worked in one place but not the others. I think it should already work as a directive (just needs a test) but not as option to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. I'll stop bike-shedding on names from now on. :)
tests/run/annotate_html.pyx
Outdated
@@ -11,6 +11,7 @@ | |||
|
|||
>>> import re | |||
>>> assert re.search('<pre .*def.* .*mixed_test.*</pre>', html) | |||
>>> assert not re.search('Complete\scythonized\scode', html) # per default no complete c code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if we ever change that text line in the output, we will almost certainly forget to adapt this test. Is there some structural element (or something in the C code) that we could look for instead? Maybe the (exact) C file version header line /* Generated by Cython
, that shouldn't normally appear in the annotated user output. We use the version header as user detectable Cython file header, so that's not going to change (ever ;-) ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first line in every annotated html is "Generated by Cython XXX" - quite close to "/* Generated by Cython ";)
I have added an explicit marker rather than using a magic value - not sure it is much better though...
Right now, parsing directives from comment line is called via I was unsure about adding it "annotate" to Maybe it is best to adjust the argument parser of |
I also have added But as support for Python2.6 is (soon) dropped, maybe it is time to replace |
Cython/Compiler/CmdLine.py
Outdated
@@ -65,6 +66,7 @@ def bad_usage(): | |||
|
|||
|
|||
def parse_command_line(args): | |||
print("parsing line!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
Yeah, seems worth a warning at some point…
Adding it to
Feel free to write a PR for that. :) |
Regarding argparse, BTW, there's a hugely old PR that is rather outdated but tried to implement a cross-Python-version CLI at the time. Probably not worth reviving but might still serve as a source of some inspiration. |
7945167
to
29adcba
Compare
@scoder I've added tests for Options.annotate (29adcba) and also for cythonize (2a81e45#diff-6210f720b78ff675f6f1c6a6be51de98). I didn't make a test for Not sure, why some tests are failing though. |
|
||
from codecs import open | ||
import os.path as os_path | ||
module_path = os_path.join(os_path.dirname(__file__), os_path.basename(__file__).split('.', 1)[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having __file__
available during module initialisation is a Py3.5+ feature based on PEP-489.
Instead, you can define a test function in this module and call it from the command section above.
Or, just use the plain file name without directory. The .html
file ends up right next to the source file.
…Python<3.5, wrap check into a function, use __name__ rather than __file__ for html-file
6606a32
to
34599c1
Compare
@scoder Thanks. I've updated the test cases, now it is green. |
Thanks! |
Another alternative to close #2855 by adding the complete cythonized code to the annotated html.
Question: Maybe there should be an option, that the whole code is added only if asked (i.e. from IPython with
--show-cythonized-code
-option)?