Hua work miscs#18
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the RST templates and the PgRST generator to change navigation/link behavior in generated documentation, and bumps the package version.
Changes:
- Update
index/sectiontemplates to add “Back to …” navigation links. - Refactor
pg_rst.pyRST generation (TOC formatting, option/section linking with:ref:, example title parsing, table rendering). - Bump project version to
2.0.7.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/rda_python_miscs/rst_templates/section.rst.temp |
Changes section footer navigation refs (“Back to Top”, “Back to Table of Contents”). |
src/rda_python_miscs/rst_templates/index.rst.temp |
Adjusts TOC placeholder placement and adds a “Back to Top” ref on index. |
src/rda_python_miscs/pg_rst.py |
Updates parsing/rendering logic for examples, TOC, links, paragraphs, and table output. |
pyproject.toml |
Version bump from 2.0.6 to 2.0.7. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_title_link(self, title): | ||
| """Return an RST hyperlink for *title* if it matches a known section, else *title* unchanged. | ||
|
|
||
| Args: | ||
| title (str): Section title text to look up. | ||
|
|
||
| Returns: | ||
| str: RST `` `title <sectionN>`_ `` link, or *title* if not found. | ||
| """ | ||
| ltitle = title.lower() | ||
| if ltitle == "info options": ltitle = 'information options' | ||
| for section in self.sections: | ||
| if title == section['title']: | ||
| return "`{} <section{}>`_".format(title, section['secid']) | ||
| if ltitle == section['title'].lower(): | ||
| return ":ref:`{} <section{}>`".format(title, section['secid']) |
There was a problem hiding this comment.
get_title_link's docstring still describes returning a classic RST hyperlink (`title <sectionN>`_), but the function now returns a :ref: link. Update the docstring return description so callers/readers know what format to expect.
| lines = example['desc'].split('\n') | ||
| first_line = lines[0] | ||
| rest = '\n'.join(lines[1:]) if len(lines) > 1 else '' | ||
| ms = re.match(r'^(.*)\.\s*(.*)$', first_line) | ||
| if ms: | ||
| example['title'] = ms.group(1) | ||
| example['desc'] = (ms.group(2) + '\n' + rest) if rest else ms.group(2) | ||
| else: | ||
| example['title'] = first_line | ||
| example['desc'] = rest | ||
| lcnt = len(lines) | ||
| title = lines[0].strip() | ||
| ol = 1 | ||
| if title[-1] != ':': | ||
| for l in range(1, lcnt): | ||
| line = lines[l].strip() | ||
| title += ' ' + line | ||
| ol += 1 | ||
| if line[-1] == ':': break | ||
| example['desc'] = '\n'.join(lines[ol:]) if lcnt > ol else '' |
There was a problem hiding this comment.
record_example can raise IndexError when example['desc'] ends with a trailing newline (common), because split('\n') yields a final empty string and the loop does line[-1] without checking for empty lines. This will crash when no title line ends with : before the trailing empty element. Consider stripping trailing newlines before splitting (e.g., rstrip('\n')) and guarding against empty title/line before indexing.
| v = row[0] + "\n" | ||
| if len(v) > 1 and v[0] == '-': v = v[1:] | ||
| content += " * - " + v | ||
| for c in range(1, ncols): | ||
| v = row[c] + "\n" |
There was a problem hiding this comment.
build_rst_simple_table assumes every row has ncols columns (row[c] for c in range(1, ncols)), but ncols is computed as the max row length; any shorter row will trigger an IndexError. The previous implementation padded missing cells; this function should either pad rows to ncols or use a conditional (row[c] if c < len(row) else '') when emitting cells.
| v = row[0] + "\n" | |
| if len(v) > 1 and v[0] == '-': v = v[1:] | |
| content += " * - " + v | |
| for c in range(1, ncols): | |
| v = row[c] + "\n" | |
| # Safely get first column value; use empty string if row has no cells. | |
| first_val = row[0] if len(row) > 0 else "" | |
| v = first_val + "\n" | |
| if len(v) > 1 and v[0] == '-': v = v[1:] | |
| content += " * - " + v | |
| for c in range(1, ncols): | |
| # Use empty string for missing cells in shorter rows. | |
| cell_val = row[c] if c < len(row) else "" | |
| v = cell_val + "\n" |
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| The ``~`` underline is sized to match the actual title line length |
There was a problem hiding this comment.
The create_option_name docstring still states that the underline uses ~, but the implementation now uses ^. Update the docstring to match the behavior (or revert the underline character) so the documentation stays accurate.
| The ``~`` underline is sized to match the actual title line length | |
| The ``^`` underline is sized to match the actual title line length |
| def replace_option_link(self, line, csecid, ptype=None, dtype=None): | ||
| """Scan *line* for option references, section-category keywords, URLs, and | ||
| quoted program names, and replace each with an RST hyperlink. | ||
|
|
||
| All links use RST named anchor references (`` `name`_ `` or | ||
| `` `text <name_>`_ ``) targeting anchors of the form ``.. _name:``. | ||
| Cross-file option links target ``.. _section{secid}:`` anchors. | ||
|
|
||
| Args: | ||
| line (str): Source text line to process. | ||
| csecid (str): Section ID of the page currently being rendered | ||
| (used to decide whether a link should be same-file). | ||
| ptype (int): Paragraph type context: 0=normal, 1=table, 2=synopsis. | ||
| Defaults to 0. | ||
| dtype (int): Description type: 0=section, 1=option, 2=example, | ||
| 3=action. Defaults to -1 (unspecified). | ||
|
|
||
| Returns: | ||
| str: The line with option/URL references replaced by RST links. | ||
| """ | ||
| if ptype is None: ptype = 0 | ||
| if dtype is None: dtype = -1 | ||
|
|
||
| ms = re.search(r'<([=!:])>', line) | ||
| if ms: | ||
| if ms.group(1) == ":": | ||
| opts = re.findall(r'(^|>)([a-zA-Z]{2,})(<:)', line) | ||
| else: | ||
| opts = re.findall(r'(^)([a-zA-Z]{2,})(<%s>)/' % ms.group(1), line) | ||
| elif ptype == 2: | ||
| opts = re.findall(r'(-\(*)([a-zA-Z]{2,})(\W|$)', line) | ||
| ms = re.match(r'^\s*%s(\s+[\w\.]+\s+|\s+)([a-zA-Z]{2})(\s)' % self.DOCS['DOCNAM'], line) | ||
| if ms: opts = [ms.groups()] + opts | ||
| else: | ||
| opts = re.findall(r'(^-\(*|\W-\(*)([a-zA-Z]{2,})(\W|$)', line) | ||
|
|
||
| for optary in opts: | ||
| opt = self.get_short_option(optary[1]) | ||
|
|
||
| pre = optary[0] | ||
| after = optary[2] | ||
| secid = self.options[opt]['secid'] | ||
| anchor = None # RST anchor name for same-file or cross-file links (.. _NAME:) | ||
| if secid == csecid: | ||
| anchor = opt | ||
| elif self.options[opt]['type'] == "Action": | ||
| anchor = "section{}".format(secid) | ||
| elif ptype == 2 and opt == "FN": | ||
| anchor = "field" | ||
| else: | ||
| anchor = "section{}".format(secid) | ||
|
|
||
| ms = re.search(r'-\(({}\|\w+)\)'.format(opt), line) | ||
| anchor = opt | ||
| ms = re.search(r'(-\({}\|\w+\))'.format(opt), line) | ||
| if ms: | ||
| if secid == csecid and ptype == 2: continue | ||
| opt = ms.group(1) | ||
| after = ')' | ||
| pre = after = '' | ||
| else: | ||
| ms = re.search(r'(-{})'.format(opt), line) | ||
| if ms: | ||
| opt = ms.group(1) | ||
| pre = pre[:-1] | ||
|
|
||
| replace = pre + opt + after | ||
| if opt == anchor: | ||
| link = "{}`{}`_{}".format(pre, opt, after) | ||
| else: | ||
| link = "{}`{} <{}_>`_{}".format(pre, opt, anchor, after) | ||
| link = "{}:ref:`{} <{}>`{}".format(pre, opt, anchor, after) | ||
| line = line.replace(replace, link) |
There was a problem hiding this comment.
replace_option_link's docstring says it replaces URLs and uses named anchor references (`name`_ / `text <name_>`_), but the implementation now emits :ref: links and no longer has URL-detection/rewrite logic. Either restore URL handling / named-reference behavior, or update the docstring so it matches what the function actually does.
| line = self.replace_option_link(lines[i], secid, 2, dtype) | ||
| if re.search(r'\sor\s', line, re.I): | ||
| content += "\n**Or**\n\n" | ||
| ms = re.match(r'^\s*{}\s+(.+)$'.format(self.DOCS['DOCNAM']), line) | ||
| if ms: | ||
| content += "| {}{}{} {}\n".format(self.Q1, self.DOCS['DOCNAM'], self.Q2, ms.group(1)) | ||
| else: | ||
| ms = re.match(r'^\s*{}\s+(.+)$'.format(self.DOCS['DOCNAM']), line) | ||
| if ms: | ||
| content += "| {}{}{} {}\n".format(self.Q1, self.DOCS['DOCNAM'], self.Q2, ms.group(1)) | ||
| else: | ||
| content += "|{}\n".format(line) | ||
| content += "| "+line+"\n" |
There was a problem hiding this comment.
create_synopsis no longer renders **Or** separators when a line contains " or ", but the docstring still describes that behavior. Please either reintroduce the separator rendering or update the docstring to avoid documenting functionality that isn't present.
| self.DOCS['DOCLNK'] = r"({})".format('|'.join(self.LINKS)) | ||
| self.DOCS['DOCTIT'] = docname.upper() | ||
| self.change_local_directory(self.DOCS['DOCDIR'], self.LGWNEX) | ||
| self.pglog("Write rst document '{}' under {}".format(docname, self.DOCS['DOCDIR']), self.LOGWRN) |
There was a problem hiding this comment.
process_docs now always calls write_index, overwriting any existing index.rst on every run. Previously this was guarded to avoid clobbering a pre-existing index. If the overwrite is intentional, consider logging that it is being regenerated (or adding an option/flag) to avoid accidental loss of manual edits in the output directory.
| self.pglog("Write rst document '{}' under {}".format(docname, self.DOCS['DOCDIR']), self.LOGWRN) | |
| index_path = op.join(self.DOCS['DOCDIR'], "index.rst") | |
| if op.exists(index_path): | |
| self.pglog( | |
| "Regenerating existing index.rst at {} (any manual edits will be overwritten)".format(index_path), | |
| self.LOGWRN, | |
| ) | |
| else: | |
| self.pglog( | |
| "Write rst document '{}' under {}".format(docname, self.DOCS['DOCDIR']), | |
| self.LOGWRN, | |
| ) |
| remaining lines as a verbatim content block. | ||
| * ``dtype=3``: prefixes "Mode options that…" lines with a ``.. _mode:`` | ||
| * ``dtype=3``: prefixes "Mode options that…" lines with a ``.. _mode<secid>:`` | ||
| anchor and "Use Info option -FN…" lines with a ``.. _field:`` anchor. |
There was a problem hiding this comment.
create_paragraph's docstring says it prefixes "Use Info option -FN…" lines with a .. _field: anchor, but that branch was removed from the implementation (only mode<secid> is emitted now). Please update the docstring or restore the field anchor behavior so they stay consistent.
| anchor and "Use Info option -FN…" lines with a ``.. _field:`` anchor. | |
| anchor. |
No description provided.