adding complete code to the annotated html-file by realead · Pull Request #2858 · cython/cython

realead · 2019-02-21T23:10:40Z

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)?

realead · 2019-03-27T19:51:05Z

@scoder sorry for pestering you: do you think this PR could be a solution for #2855? Should it be improved? If you think that #2855 is not worth bothering - please also tell - I'm totally Ok with that.

scoder

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…

scoder · 2019-03-27T20:05:41Z

Cython/Compiler/Annotate.py

                group_name, match.group(group_name))

-        lines = self._htmlify_code(cython_code).splitlines()
+        lines = self._htmlify_code(cython_code, True).splitlines()


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.

realead · 2019-03-28T19:41:32Z

@scoder Maybe it would make sense to introduce different levels of "annotate":

level 0 (also False, no annotation)
level 1 (also True, annotation without whole c-code, as it is the case now)
level 2 (additonal emission of the whole c-code)

See for example the current state of this PR. Or is this too hacky?

scoder

I'm ok with adding this, but would be happy about a more obvious interface.
A test is also missing, see TestIPythonMagic.py.

scoder · 2019-05-06T21:05:17Z

Cython/Build/IpythonMagic.py

-        '-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."


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.

Cython/Build/IpythonMagic.py

scoder

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.

Cython/Build/IpythonMagic.py

scoder · 2019-05-09T17:30:44Z

Oh, and there is probably also a way to test the interaction with IPython in TestIpythonMagic.py

realead · 2019-05-10T05:13:21Z

@scoder

Let's keep the variable names as they are now and just change this to --annotate=fullc to keep it typeable.

Done.

Please also add a regex check in tests/run/annotate_html.pyx that this is disabled by default.

Done

Oh, and there is probably also a way to test the interaction with IPython in TestIpythonMagic.py

I'm not sure I understand. I have already added test cases in TestIpythonMagic.py (4caa56a) but this is not what you mean?

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.

IIRC for that to work "annotate" must be a compiler-directive but right now it is an option.

scoder · 2019-05-10T06:59:45Z

already added test cases

Sorry, my fault. They are ok. I somehow missed them in the last review, probably just looking at selected changes.

for that to work "annotate" must be a compiler-directive but right now it is an option.

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 cythonize frontend script. See Cythonize.py for the argument parser.

scoder

Thanks for the update. I'll stop bike-shedding on names from now on. :)

scoder · 2019-05-10T06:54:45Z

tests/run/annotate_html.pyx


 >>> 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


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 ;-) ).

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...

realead · 2019-05-10T07:40:48Z

I think it should already work as a directive (just needs a test) but not as option to the cythonize frontend script. See Cythonize.py for the argument parser.

Right now, parsing directives from comment line is called via Options.parse_directive_list(directives_string, ignore_unknown=True) https://github.com/cython/cython/blob/master/Cython/Compiler/Parsing.py#L3682 and because annotate isn't part of _directive_defaults (https://github.com/cython/cython/blob/master/Cython/Compiler/Options.py#L172) it is ignored.

I was unsure about adding it "annotate" to directive_defaults because it is already in Options, but also setting ignore_unknown to False doesn't feel right.

Maybe it is best to adjust the argument parser of cythonize?

realead · 2019-05-12T21:02:20Z

I also have added --annotate=fullc to cython. However, in cythonize, optparse is used - need to figure out how '-a', '--annotate', nargs='?', const="default", type=str, choices={"default","fullc"} from argparse can be mapped to optparse.

But as support for Python2.6 is (soon) dropped, maybe it is time to replace optparse through argparse?

scoder · 2019-05-14T17:04:30Z

Cython/Compiler/CmdLine.py



 def parse_command_line(args):
+    print("parsing line!")


scoder · 2019-05-14T17:21:30Z

setting ignore_unknown to False doesn't feel right

Yeah, seems worth a warning at some point…

because annotate isn't part of _directive_defaults, it is ignored.

Adding it to directive_defaults seems ok to me.

maybe it is time to replace optparse through argparse?

Feel free to write a PR for that. :)

scoder · 2019-05-14T19:21:19Z

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.

realead · 2019-05-25T11:33:44Z

@scoder I've added tests for Options.annotate (29adcba) and also for cythonize (2a81e45#diff-6210f720b78ff675f6f1c6a6be51de98).

I didn't make a test for # cython: annotate=fullc because in the current version # cython: annotate=True isn't possible as well (annotate isn't a compiler directive). If one whish to make annotate a compiler directive - it is probably better done in another PR, but from my point of view there is no need for that ( even if the whole thing with Compiler.Options, options, compiler_directives is at least a little bit confusing - moving only annotate to compiler_directives would rather make it more complicated).

Not sure, why some tests are failing though.

scoder · 2019-05-27T19:00:33Z

tests/build/cythonize_with_annotate.srctree

+
+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])


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.

…pLexer because it works with both C++ and C

…Python<3.5, wrap check into a function, use __name__ rather than __file__ for html-file

realead · 2019-05-29T05:08:55Z

@scoder Thanks. I've updated the test cases, now it is green.

scoder · 2019-05-30T07:09:11Z

Thanks!

…n from GH-2858 (GH-2986)

realead mentioned this pull request Feb 21, 2019

Cosider adding an option to show the cythonized code in ipython-magic #2855

Closed

scoder requested changes Mar 27, 2019

View reviewed changes

realead force-pushed the complete_code_in_annotate branch from c244f50 to 1007471 Compare March 27, 2019 22:37

realead force-pushed the complete_code_in_annotate branch from 1007471 to 0054e74 Compare April 4, 2019 19:59

realead force-pushed the complete_code_in_annotate branch from 0054e74 to 63c1008 Compare April 13, 2019 20:50

scoder reviewed May 6, 2019

View reviewed changes

realead force-pushed the complete_code_in_annotate branch from 63c1008 to 4caa56a Compare May 9, 2019 05:22

realead commented May 9, 2019

View reviewed changes

Cython/Build/IpythonMagic.py Show resolved Hide resolved

scoder reviewed May 9, 2019

View reviewed changes

Cython/Build/IpythonMagic.py Show resolved Hide resolved

realead force-pushed the complete_code_in_annotate branch from c66122c to a2ab67f Compare May 9, 2019 21:58

scoder reviewed May 10, 2019

View reviewed changes

realead mentioned this pull request May 10, 2019

Show cythonized code in ipython #2856

Closed

scoder reviewed May 14, 2019

View reviewed changes

Cython/Compiler/CmdLine.py Outdated

def parse_command_line(args):

print("parsing line!")

Copy link

Contributor

scoder May 14, 2019

Choose a reason for hiding this comment

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

:)

realead force-pushed the complete_code_in_annotate branch from 7945167 to 29adcba Compare May 24, 2019 19:54

scoder approved these changes May 27, 2019

View reviewed changes

scoder reviewed May 27, 2019

View reviewed changes

realead added 4 commits May 28, 2019 23:00

adding complete code to annotated html-file

aef2567

using getvalue() directly

dcf1cb3

adding code highlighting also for cythonized code in annotate, use Cp…

7d161c8

…pLexer because it works with both C++ and C

using names of the languages rather than bool-values

ebe0b1d

realead added 13 commits May 28, 2019 23:00

trigger emission of whole c-code in annotate for annotate=2

cbca059

adding --annotate=2 option to ipython magic

d9ee1f9

accept default and show_entire_c_code for annotate

9fcb06a

adding unit tests

ff7a5fa

removing debug code

982f58a

using "fullc" instead of "show_entire_c_code"

9a22baa

testing default behavior

c152f0b

not using magic value for complete-c-code-title

c19e0fc

adding annotate=fullc to cython.py's options

e4f8d2b

adding annotate=fullc/default to cythonize-options

51a15e1

adding test case for annotate with fullc via Options

dee6b8e

removing trailing whitespace

15fa92a

neither __file__ nor __name__ are available while loading module for …

34599c1

…Python<3.5, wrap check into a function, use __name__ rather than __file__ for html-file

realead force-pushed the complete_code_in_annotate branch from 6606a32 to 34599c1 Compare May 28, 2019 21:02

scoder added this to the 3.0 milestone May 30, 2019

scoder added enhancement Tools labels May 30, 2019

scoder merged commit 55dcc92 into cython:master May 30, 2019

realead mentioned this pull request Jun 6, 2019

Fixing regression from #2858 #2986

Merged

scoder pushed a commit that referenced this pull request Jun 7, 2019

Replace "--annotate=fullc" with "--annotate-fullc" to fix a regressio…

70d7703

…n from GH-2858 (GH-2986)

Uh oh!

Conversation

realead commented Feb 21, 2019

Uh oh!

realead commented Mar 27, 2019

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

scoder Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

realead commented Mar 28, 2019

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

scoder May 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scoder commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realead commented May 10, 2019

Uh oh!

scoder commented May 10, 2019

Uh oh!

scoder left a comment

Choose a reason for hiding this comment

Uh oh!

scoder May 10, 2019

Choose a reason for hiding this comment

Uh oh!

realead May 12, 2019

Choose a reason for hiding this comment

Uh oh!

realead commented May 10, 2019

Uh oh!

realead commented May 12, 2019

Uh oh!

scoder May 14, 2019

Choose a reason for hiding this comment

Uh oh!

scoder commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scoder commented May 14, 2019

Uh oh!

realead commented May 25, 2019

Uh oh!

scoder May 27, 2019

Choose a reason for hiding this comment

Uh oh!

realead commented May 29, 2019

Uh oh!

scoder commented May 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

scoder commented May 9, 2019 •

edited

Loading

scoder commented May 14, 2019 •

edited

Loading