Bmedx/docgen#29
Conversation
|
|
||
| Args: | ||
| report_filename: Filename to write to. | ||
| report_data: Dict of reporting data to use, in the {'file name': [list, of, annotations,]} style. |
There was a problem hiding this comment.
FWIW, I got stuck on understanding "report objects" (report_data here) for a while because I didn't realize the filename keys were names of source files (rather than report files). More verbage around this would be helpful.
Also, shouldn't report_filename/full_report_filename actually be doc_filename/full_doc_filename because it's an rst doc (rather than a yaml report)?
| @@ -0,0 +1,5 @@ | |||
| {% if annotation.extra and annotation.extra.object_id %} | |||
| `<{{ annotation.extra.object_id }}> line {{ annotation.line_number }} <{{ source_link_prefix }}{{ filename }}#L{{ annotation.line_number }}>`_: {{ annotation.annotation_token }} {% include "annotation_data.tpl" %} | |||
There was a problem hiding this comment.
Links to the exact line on github!? 🆒 🥇
| {% else %} | ||
| {% if loop.changed(annotation.report_group_id) %} | ||
|
|
||
| {% endif %} |
There was a problem hiding this comment.
Can you clarify what the above if-block does exactly? Is the extra newline necessary to terminate the group because un-indenting isn't enough to cue the termination?
There was a problem hiding this comment.
Yes, unfortunately handling rst whitespace in Jinja2 templates is super finicky and trying to comment them only makes it worse.
| @@ -1,7 +1,11 @@ | |||
| source_path: ../ | |||
| source_path: ../devstack-docker/edx-platform/ | |||
There was a problem hiding this comment.
was this change intentional, or temporary for testing?
| """ | ||
| safelist_file.write(safelist_comment.lstrip()) | ||
| yaml_ordered_dump(safelist_data, stream=safelist_file, default_flow_style=False) | ||
| yaml.safe_dump(safelist_data, stream=safelist_file, default_flow_style=False) |
There was a problem hiding this comment.
I'm also just generally unclear as to why these don't need to be ordered anymore.
There was a problem hiding this comment.
Oddly sorted dicts seem to be the default behavior in pyyaml so this seems to have not been necessary in the first place: https://github.com/yaml/pyyaml/blob/4c2e993321ad29a02a61c4818f3cef9229219003/lib3/yaml/representer.py#L108-L114
I checked this in Python 3.6 and 2.7:
>>> a = {'z':1, 'g':2, 'a': 3, 'b': 4, 'w': 5}
>>> import yaml
>>> yaml.safe_dump(a)
'{a: 3, b: 4, g: 2, w: 5, z: 1}\n'
I found it because the new edx-lint enforces yaml.safe_load/dump, which fails on the old OrderedDict approach.
| @@ -1,7 +1,11 @@ | |||
| source_path: ../ | |||
| source_path: ../devstack-docker/edx-platform/ | |||
There was a problem hiding this comment.
Did this path sneak in? Or was it intended?
| for report_annotation in report[filename]: | ||
| if loaded_annotation['line_number'] == report_annotation['line_number'] and \ | ||
| loaded_annotation['annotation_token'] == report_annotation['annotation_token'] and \ | ||
| loaded_annotation['annotation_data'] == report_annotation['annotation_data']: |
There was a problem hiding this comment.
Is this any better?:
index_keys = ('line_number', 'annotation_token', 'annotation_data')
if all( [ loaded_annotation[k] == report_annotation[k] for k in index_keys ] ):
| @@ -0,0 +1,8 @@ | |||
| {% if annotation.annotation_data is sequence and annotation.annotation_data is not string %} | |||
| {% for a in annotation.annotation_data %} | |||
| choice_{{ slugify(a) }}_{% if not loop.last %}, {% endif %} | |||
There was a problem hiding this comment.
What is this loop magic? I'm intrigued...
There was a problem hiding this comment.
Nm - I found it:
http://jinja.pocoo.org/docs/2.10/templates/#for
| file_extension: All files with this extension will be deleted | ||
| """ | ||
| try: | ||
| filelist = [f for f in os.listdir('test_reports') if f.endswith(file_extension)] |
There was a problem hiding this comment.
Nit: Any reason for this line to be in the try...except block instead of outside it?
|
|
||
| def _do_find(source_path, new_report_path): | ||
| """ | ||
| Do a static annotation search with report, rename the report to a distinct name, return the new name. |
There was a problem hiding this comment.
It doesn't return the new name anymore?
- Adds ability to turn yaml reports into .rst - Uses Jinja2 templates, so open to extension - Can combine multiple report files - Changes reporting format to use safe_read / safe_write - Can create direct links to Github
|
@pwnage101 @doctoryes this is ready for re-review |
4 participants
Description: Adds the ability to generate human readable reports in RST from code-annotations generated YAML files.
JIRA: PLAT-2359
Author concerns: There is some basic functionality here for being able to use other templates to potentially create other types of output (HTML, MD, etc.) but it's pretty half baked. I'd like to come back to that and clean that up later, and add HTML generation as a proof-of-concept. Maybe next hackathon...