Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upSome proposed changes to pp_diagram output, plus monkeypatch create_diagram method onto ParserElement #236
Conversation
Pull latest from pyparsing/pyparsing
…gram method, demo in delta_time.py
| def _already_extracted( | ||
| element: pyparsing.ParserElement, lookup: ConverterState | ||
| ) -> bool: | ||
| return any(element.name == diag.kwargs["name"] for diag in lookup.diagrams.values()) |
TMiguelT
Aug 17, 2020
Contributor
The ConverterState object indexes both of its dictionaries using the pyparsing element ID. So you can just check return id(element) in lookup.diagrams here.
The ConverterState object indexes both of its dictionaries using the pyparsing element ID. So you can just check return id(element) in lookup.diagrams here.
ptmcg
Aug 17, 2020
Author
Member
Ok, much simpler!
Ok, much simpler!
| @@ -341,9 +354,9 @@ def _to_diagram_element( | |||
| ret = EditablePartial.from_call(railroad.Sequence, items=[]) | |||
| elif isinstance(element, (pyparsing.Or, pyparsing.MatchFirst)): | |||
| if _should_vertical(vertical, len(exprs)): | |||
| ret = EditablePartial.from_call(railroad.HorizontalChoice, items=[]) | |||
TMiguelT
Aug 17, 2020
Contributor
Good catch, I evidently had this wrong. Let me know if it actually looks better this way, though.
Good catch, I evidently had this wrong. Let me know if it actually looks better this way, though.
| railroad.Terminal, title=element.name, text=element.defaultName | ||
| ) | ||
| else: | ||
| ret = EditablePartial.from_call( |
TMiguelT
Aug 17, 2020
Contributor
I think this check (if not _already_extracted) could be simplified just by adding a new condition to _worth_extracting. As it stands, we already check if it's _worth_extracting, and if it has already been extracted, then I already have code for creating a NonTerminal here:
pyparsing/pyparsing/diagram/__init__.py
Lines 332 to 345
in
1b86c49
.
So if you simply add if _is_named_terminal(element): return True to _already_extracted, I think you get this behaviour for free, with less and simpler code.
I think this check (if not _already_extracted) could be simplified just by adding a new condition to _worth_extracting. As it stands, we already check if it's _worth_extracting, and if it has already been extracted, then I already have code for creating a NonTerminal here:
pyparsing/pyparsing/diagram/__init__.py
Lines 332 to 345 in 1b86c49
So if you simply add if _is_named_terminal(element): return True to _already_extracted, I think you get this behaviour for free, with less and simpler code.
| ) | ||
| else: | ||
| ret = EditablePartial.from_call( | ||
| railroad.NonTerminal, title=element.name, text=element.name |
TMiguelT
Aug 17, 2020
Contributor
title will only set the hover text, I assume you know that? This will make it harder to see both the customName and the defaultName compared to my current solution of using a group to do that:
pyparsing/pyparsing/diagram/__init__.py
Lines 387 to 393
in
1b86c49
title will only set the hover text, I assume you know that? This will make it harder to see both the customName and the defaultName compared to my current solution of using a group to do that:
pyparsing/pyparsing/diagram/__init__.py
Lines 387 to 393 in 1b86c49
ptmcg
Aug 17, 2020
Author
Member
The default name gets displayed in the element's subdiagram, so we shouldn't need to show both defaultName and customName at the same time.
The default name gets displayed in the element's subdiagram, so we shouldn't need to show both defaultName and customName at the same time.
TMiguelT
Aug 17, 2020
Contributor
Oh, okay. I think I need to see an example of this.
Oh, okay. I think I need to see an example of this.
| @@ -379,6 +401,12 @@ def _to_diagram_element( | |||
| number=lookup.generate_index(), | |||
| ) | |||
|
|
|||
| if _is_named_terminal(element): | |||
TMiguelT
Aug 17, 2020
Contributor
So here you're bypassing the normal mechanism of "only extract if we've seen it twice". Is this intentional? I forget what we discussed. If so, there's probably an easier way to do this than using the extraction mechanism at all, but I can't think of exactly what it might be.
So here you're bypassing the normal mechanism of "only extract if we've seen it twice". Is this intentional? I forget what we discussed. If so, there's probably an easier way to do this than using the extraction mechanism at all, but I can't think of exactly what it might be.
ptmcg
Aug 17, 2020
Author
Member
Yeah, I think this is intentional. Even if an object is only seen once, if it is sufficiently complex (and has a name), it should be a separate subdiagram. This makes the diagrams more likely to map to a BNF.
Yeah, I think this is intentional. Even if an object is only seen once, if it is sufficiently complex (and has a name), it should be a separate subdiagram. This makes the diagrams more likely to map to a BNF.
|
|
||
|
|
||
| # monkeypatch .create_diagram method onto ParserElement | ||
| def _create_diagram(expr: pyparsing.ParserElement, output_html: Union[str, TextIO]): |
TMiguelT
Aug 17, 2020
Contributor
I think this might be better as a method on the original ParserElement class, which checks for an ImportError as you have done at the top. I prefer this to monkey patching, anyway.
I think this might be better as a method on the original ParserElement class, which checks for an ImportError as you have done at the top. I prefer this to monkey patching, anyway.
ptmcg
Aug 17, 2020
Author
Member
My intention behind monkeypatching is so that the core library can be easily vendored without having to include dependencies on other packages beside the stdlib.
My intention behind monkeypatching is so that the core library can be easily vendored without having to include dependencies on other packages beside the stdlib.
TMiguelT
Aug 17, 2020
Contributor
But if you do this, is that not sufficient? Then the dependencies are still optional.
try:
import pyparsing.diagram
except ImportError:
raise Exception('You need to run `pip install pyparsing[diagrams]`')
But if you do this, is that not sufficient? Then the dependencies are still optional.
try:
import pyparsing.diagram
except ImportError:
raise Exception('You need to run `pip install pyparsing[diagrams]`')
@TMiguelT - I've had these changes for a while, but wasn't sure about the Git incantations to push them properly. I think this is... oh, crap, I should have done this in a branch. Well, I can just nuke this forked repo when we are done, rather than try to patch it up.
These changes are not complete, there are still some issues with some of the labeling I think, and proper referencing to a named element if it is used multiple times in a high expression.
I also would like to add the results name to the dashed border now, since the expression name is getting written on the block element itself. If you can point me to where this is set, I'll take a run at it.
What do you think of monkeypatching
.create_diagramontoParserElement?