Refactor attributes and improve argument parsing #8
Conversation
mudge
commented
Aug 21, 2024
mudge
commented
- Remove redundant stripping of whitespace
- Expand usage of argparse to handle CLI options
- Refactor attribute handling
Comparing the resulting AsciiDoc with and without this extra stripping of trailing whitespace on the current Raspberry Pi Pico C/C++ SDK databook results in no change in output.
mudge
force-pushed
the
refactoring
branch
2 times, most recently
from
f048b6f to
372d99b
Compare
| attrlist = [] | ||
|
|
||
| if "id" in attributes: | ||
| attrlist.append(f"#{sanitize(attributes.pop('id'))}") |
There was a problem hiding this comment.
Haha, I was about to suggest that it might be worth making attrlist a dict instead of a list, but then I spotted that this first item isn't of the form "xxx=yyy".
There was a problem hiding this comment.
If we can break the dependency on matching the role=h6 contextspecific substring, we can adopt a more concise syntax here and use the AsciiDoc shorthand:
[#group_foo.h6.contextspecific,tag=PICO_2040,type=PICO_2040]
|
👍 to keeping things as dicts/lists for as long as possible, rather than prematurely converting to strings. |
Rather than using optional string arguments for both input and output files,
use more of Python's argparse library to handle a single, mandatory,
positional file argument (which now returns a handle to read the file).
This is a breaking change as the "-f" argument is now a positional one
so any downstream usage will need to be updated.
Note we don't use `argparse.FileType("w")` for the output file as that
would cause the given file to be created or truncated as soon as the
program runs when we only want it to produce output if successful.
Rather than passing around an AsciiDoc string of attributes, pass around a dictionary that is only converted to AsciiDoc at the very end when passed to the `title` helper. Note I considered switching to using the role shorthand for multiple roles (e.g. `.h6.contextspecific` rather than `role=h6 contextspecific`) but there is downstream code that relies on matching the existing format. Similarly, the order of the `tag` and `type` attributes is currently relied upon by client code so these have to be special cased for now. This also removes the dedicated Title Node class in favour of handling title rendering in the parent element (in this case, the Sect Node).
Rather than joining tuples on the empty string, use f-strings to build up simple strings with interpolated expressions.
nodes.py
Outdated
|
|
||
|
|
||
| class SimplesectNode(Node): | ||
| def to_asciidoc(self, **kwargs): | ||
| previous_node = self.previous_node() | ||
| next_node = self.next_node() | ||
| if self.node.get("kind", "") == "see": | ||
| if self.node.get("kind") == "see": |
There was a problem hiding this comment.
Looks like lines 665 and 668 could be converted from if to elif ? And given that they're all doing self.node.get("kind") perhaps its worth doing kind = self.node.get("kind") above line 648?
There was a problem hiding this comment.
As there is an early return on line 666 😈, pylint will complain if you switch to an elif:
Unnecessary "elif" after "return", remove the leading "el" from "elif"
There was a problem hiding this comment.
Memoized self.node.get("kind") in 06370ed
Rather than repeatedly loading the kind from the node's attributes, fetch it once and then switch on the resulting value multiple times.
