Push v0.3.3 to main #20
Conversation
Summary by CodeRabbit
WalkthroughThis update introduces several significant changes across the codebase. The GitHub Actions workflow is modified to remove the step that publishes distributions to TestPyPI, retaining only the main PyPI publishing step. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InnerHTML
participant Symbol
User->>InnerHTML: find(key) / get_by_text(text) / get_by_attr(attr, value) / advanced_find(tag, attrs)
InnerHTML->>InnerHTML: Look up elements in attrs/text indexes
InnerHTML->>Symbol: Return list of matching Symbol instances
sequenceDiagram
participant User
participant Table
User->>Table: to_dict()
Table->>Table: Convert columns to dict of header to cell lists
Table-->>User: Return dictionary
User->>Table: from_dict(data)
Table->>Table: Create Table from dict, populate head/body
Table-->>User: Return Table instance
Possibly related PRs
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
BetterMD/elements/style.py (1)
66-77: 🛠️ Refactor suggestionConstructor/docstring mismatch and missing
innerforwarding
inneris no longer accepted yet it is still documented (lines 70-75) and not passed to
Symbol.__init__, so callers cannot supply child elements.-def __init__(self, *, style: t.Optional[StyleDict] = None, raw: str = "", **props): +def __init__(self, *, style: t.Optional[StyleDict] = None, raw: str = "", + inner: list[Symbol] | None = None, **props): - super().__init__(**props) + super().__init__(inner=inner, **props)Please also update the docstring to drop/reflect the changed parameters.
BetterMD/elements/symbol.py (1)
72-82:⚠️ Potential issue
copy()uses undefined variables – raisesNameError
innerandclasseswere removed from the signature but are still referenced,
andSymbol(...)is called with an obsolete positional pattern.-def copy(self, styles: dict[str, str] | None = None): - if inner is None: - inner = [] - if styles is None: - styles = {} - if classes is None: - classes = [] - - styles.update(self.styles) - return Symbol(styles, classes, inner=inner) +def copy(self, *, styles: dict[str, str] | None = None, + inner: list["Symbol"] | None = None, + **extra_props): + styles = {**self.styles, **(styles or {})} + inner = list(inner or self.children) # shallow copy by default + return type(self)(inner=inner, style=styles, **self.props, **extra_props)Without this patch
copy()raises immediately.🧰 Tools
🪛 Ruff (0.8.2)
73-73: Undefined name
inner(F821)
77-77: Undefined name
classes(F821)
🧹 Nitpick comments (5)
BetterMD/elements/symbol.py (1)
52-59:styles/classesare read-only views – provide setters or clarify immutabilityThe helper properties return the underlying mapping/list, but there is no
corresponding setter. Users mutatingsymbol.styles["color"]="red"will modify
propssilently, whereassymbol.styles = {...}will raiseAttributeError.
Consider exposing explicit getters/setters or documenting the intended usage.BetterMD/elements/document.py (2)
44-46: Incorrect class name inHashableDict.__repr__The
__repr__claims HashableList, should be HashableDict – aids debugging.- def __repr__(self): - return f"HashableList({self.dct})" + def __repr__(self): + return f"HashableDict({self.dct})"
5-5: Unused import flagged by Ruff
ATTR_TYPESis imported only for type checking; silence Ruff with a noqa:-from ..typing import ATTR_TYPES +from ..typing import ATTR_TYPES # noqa: F401🧰 Tools
🪛 Ruff (0.8.2)
5-5:
..typing.ATTR_TYPESimported but unusedRemove unused import:
..typing.ATTR_TYPES(F401)
BetterMD/elements/table.py (2)
191-200: Type annotations forcolsmay break static type-checkers
defaultdict[Th|Td|HeadlessTd, list[Td | Th]]uses PEP 604 unions directly in a sub-script position, whichmypy,pyrightand older versions oftypingreject.
Prefer a singletyping.Union(or stringified forward ref) to keep the code type-checker-friendly:- self.cols: 'defaultdict[Th|Td|HeadlessTd, list[Td | Th]]' = defaultdict(list) + self.cols: 'defaultdict[t.Union[Th, Td, HeadlessTd], list[t.Union[Td, Th]]]' = defaultdict(list)
427-433: Exception re-raising loses original traceback
raise einside theexceptclause discards the original traceback. Use exception chaining:-except Exception as e: - logger.error(f"Exception occurred in `from_list`: {e}") - raise e +except Exception as e: + logger.error("Exception occurred in `from_list`", exc_info=True) + raise e from NoneThis also satisfies Ruff B904.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/publish.yml(0 hunks)BetterMD/__init__.py(7 hunks)BetterMD/elements/col.py(1 hunks)BetterMD/elements/document.py(5 hunks)BetterMD/elements/style.py(2 hunks)BetterMD/elements/symbol.py(5 hunks)BetterMD/elements/table.py(9 hunks)BetterMD/elements/track.py(1 hunks)BetterMD/typing.py(1 hunks)setup.py(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/publish.yml
🧰 Additional context used
🧬 Code Graph Analysis (1)
BetterMD/elements/document.py (1)
BetterMD/elements/symbol.py (4)
text(66-70)get_prop(242-246)classes(57-58)inner_html(263-266)
🪛 Ruff (0.8.2)
BetterMD/elements/document.py
5-5: ..typing.ATTR_TYPES imported but unused
Remove unused import: ..typing.ATTR_TYPES
(F401)
66-66: Do not perform function call Copy in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
157-157: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
BetterMD/elements/table.py
344-344: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (7)
BetterMD/elements/col.py (1)
16-16: Improved element type definitionReplacing the boolean
self_closingattribute with a more descriptivetype = "void"improves code readability and standardises element typing across the codebase. This change aligns well with modern HTML semantics.BetterMD/typing.py (1)
3-3: Enhanced type system with recursive structure supportThe updated
ATTR_TYPEStype alias now properly supports arbitrarily nested lists and dictionaries, which is an excellent improvement. Using string literals for forward references is the correct approach for recursive type definitions in Python.setup.py (1)
3-3: Version bump for release 0.3.3The version increment from 0.3.2 to 0.3.3 follows semantic versioning practices and correctly reflects the nature of the changes in this update.
BetterMD/elements/track.py (2)
7-7: Trailing whitespace removedGood code hygiene removing unnecessary whitespace.
9-9: Improved element type definitionReplacing the boolean
self_closingattribute with a more descriptivetype = "void"improves code readability and standardises element typing across the codebase. This change aligns with the same pattern used in other void element classes.BetterMD/__init__.py (1)
70-70: Possible breaking change in__all__export listReplacing
"Symbol"with"elements"alters the public interface – callers doing
from BetterMD import Symbolwill now break.
If backwards-compatibility is required, keep exportingSymbolfor at least one minor version:-__all__ = ["HTML", "MD", "elements", "Collection", ...] +from .elements import Symbol # re-export for BC +__all__ = ["HTML", "MD", "elements", "Symbol", "Collection", ...]BetterMD/elements/table.py (1)
588-589: Column width calculation can overflow on empty cells
max(("" if col is None else col.data).splitlines(), key=len, default="")fails whencolis aTd/ThwhosedataisNone.
Safeguard withstr(col.data or "")to avoidTypeError.Would you confirm whether
Data.dataever returnsNone? If so, I'll propose a concrete patch.
| return self.attrs.get(attr, {}).get(value, []) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
get_by_attr misses hash-normalisation
Look-ups with list/dict values will fail because keys are stored in their
hashable wrappers. Apply make_hashable before querying:
-def get_by_attr(self, attr: str, value: str):
- return self.attrs.get(attr, {}).get(value, [])
+def get_by_attr(self, attr: str, value):
+ key = value if isinstance(value, (str, int, float, bool)) else make_hashable(value)
+ return self.attrs.get(attr, {}).get(key, [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return self.attrs.get(attr, {}).get(value, []) | |
| def get_by_attr(self, attr: str, value): | |
| key = value if isinstance(value, (str, int, float, bool)) else make_hashable(value) | |
| return self.attrs.get(attr, {}).get(key, []) |
| # Normalize keys in elm.props for attrs merging | ||
| normalized_props = { | ||
| prop: {make_hashable(value): [elm] for value in values} | ||
| for prop, values in elm.props.items() | ||
| } | ||
|
|
There was a problem hiding this comment.
normalized_props comprehension breaks for non-iterable attribute values
When a prop value is a scalar (str, int, bool), iterating over it yields
single characters/digits, producing nonsense keys.
Consider:
-normalized_props = {
- prop: {make_hashable(value): [elm] for value in values}
- for prop, values in elm.props.items()
-}
+normalized_props: dict[str, dict[HASHABLE_ATTRS, list[Symbol]]] = {}
+for prop, val in elm.props.items():
+ key = make_hashable(val)
+ normalized_props.setdefault(prop, {})[key] = [elm]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Normalize keys in elm.props for attrs merging | |
| normalized_props = { | |
| prop: {make_hashable(value): [elm] for value in values} | |
| for prop, values in elm.props.items() | |
| } | |
| # Normalize keys in elm.props for attrs merging | |
| normalized_props: dict[str, dict[HASHABLE_ATTRS, list[Symbol]]] = {} | |
| for prop, val in elm.props.items(): | |
| key = make_hashable(val) | |
| normalized_props.setdefault(prop, {})[key] = [elm] |
| def __init__(self, data: 't.Union[GetProtocol[T1, T2], dict[T1, T2]]', default:'T5'=Copy(None)): | ||
| self.data = data | ||
| self.default = default.copy() if isinstance(default, CopyProtocol) else default | ||
|
|
There was a problem hiding this comment.
Avoid calling Copy() in default argument (B008)
Default arguments are evaluated at import time. Replace with None and
initialise inside the function to prevent accidental state sharing.
-def __init__(self, data: ..., default: T5 = Copy(None)):
+def __init__(self, data: ..., default: T5 | None = None):
self.data = data
- self.default = default.copy() if isinstance(default, CopyProtocol) else default
+ default = default or Copy(None)
+ self.default = default.copy() if isinstance(default, CopyProtocol) else defaultCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
66-66: Do not perform function call Copy in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
| def advanced_find(self, tag:'str', attrs:'dict[t.Literal["text"] | str, str | bool | int | float | tuple[str, str | bool | int | float] | list[str | bool | int | float | tuple[str, str | bool | int | float]]]' = {}): | ||
| def check_attr(e:'Symbol', k:'str', v:'str | bool | int | float | tuple[str, str | bool | int | float]'): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mutable default for attrs parameter (B006)
Using {} as a default value causes state to leak between calls.
-def advanced_find(self, tag: str,
- attrs: dict[...]= {}):
+def advanced_find(self, tag: str,
+ attrs: dict[...]|None = None):
...
- if "text" in attrs:
+ attrs = attrs or {}
+ if "text" in attrs:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def advanced_find(self, tag:'str', attrs:'dict[t.Literal["text"] | str, str | bool | int | float | tuple[str, str | bool | int | float] | list[str | bool | int | float | tuple[str, str | bool | int | float]]]' = {}): | |
| def check_attr(e:'Symbol', k:'str', v:'str | bool | int | float | tuple[str, str | bool | int | float]'): | |
| def advanced_find(self, tag: str, | |
| attrs: dict[t.Literal["text"] | str, | |
| str | bool | int | float | | |
| tuple[str, str | bool | int | float] | | |
| list[str | bool | int | float | | |
| tuple[str, str | bool | int | float]]] | |
| | None = None): | |
| attrs = attrs or {} | |
| def check_attr(e: 'Symbol', | |
| k: 'str', | |
| v: 'str | bool | int | float | | |
| tuple[str, str | bool | int | float]'): | |
| ... |
🧰 Tools
🪛 Ruff (0.8.2)
157-157: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
| @classmethod | ||
| def from_dict(cls, data:'dict[str, list[str]]'): | ||
| self = cls() | ||
| head = THead.from_list(list(data.keys())) | ||
| body = TBody.from_list(list(data.values())) | ||
|
|
||
| self.head = head | ||
| self.body = body | ||
|
|
||
| self.add_child(head) | ||
| self.add_child(body) | ||
|
|
||
| return self |
There was a problem hiding this comment.
from_dict transposes rows/columns incorrectly
dict.values() returns columns, but TBody.from_list() expects a list of rows.
This produces a body whose row count equals the original column count.
- body = TBody.from_list(list(data.values()))
+ # Flip columns → rows
+ body = TBody.from_list(list(map(list, zip(*data.values()))))Also, consider preparing the table before returning:
self.prepare()| def from_list(cls, lst:'list[list[list[str] | str]]'): | ||
| logger.debug(f"Creating Table from list of lists with shape {len(lst)}") | ||
| self = cls() | ||
| head = THead.from_list(lst[0]) | ||
| body = TBody.from_list(lst[1]) | ||
| foot = TFoot.from_list(lst[2]) | ||
|
|
||
| self.head = head | ||
| self.body = body | ||
| self.foot = foot | ||
|
|
||
| return self | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
from_list is an instance method but behaves like a classmethod
Inside Table, def from_list(cls, …) is defined without the @classmethod decorator, so cls will actually be self.
Any call such as Table.from_list(lst) raises TypeError (“missing positional argument”).
- def from_list(cls, lst: 'list[list[list[str] | str]]'):
+ @classmethod
+ def from_list(cls, lst: 'list[list[list[str] | str]]'):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def from_list(cls, lst:'list[list[list[str] | str]]'): | |
| logger.debug(f"Creating Table from list of lists with shape {len(lst)}") | |
| self = cls() | |
| head = THead.from_list(lst[0]) | |
| body = TBody.from_list(lst[1]) | |
| foot = TFoot.from_list(lst[2]) | |
| self.head = head | |
| self.body = body | |
| self.foot = foot | |
| return self | |
| @classmethod | |
| def from_list(cls, lst: 'list[list[list[str] | str]]'): | |
| logger.debug(f"Creating Table from list of lists with shape {len(lst)}") | |
| self = cls() | |
| head = THead.from_list(lst[0]) | |
| body = TBody.from_list(lst[1]) | |
| foot = TFoot.from_list(lst[2]) | |
| self.head = head | |
| self.body = body | |
| self.foot = foot | |
| return self |
|
|
||
| def to_dict(self): | ||
| return { | ||
| k.data: [d.data for d in v] for k, v in self.cols.items() | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
to_dict() is unusable on an un-prepared table
self.cols is only populated during prepare(). Calling Table.to_dict() before the table is prepared silently returns an empty dict.
def to_dict(self):
- return {
- k.data: [d.data for d in v] for k, v in self.cols.items()
- }
+ if not self.prepared:
+ self.prepare()
+ return {k.data: [d.data for d in v] for k, v in self.cols.items()}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def to_dict(self): | |
| return { | |
| k.data: [d.data for d in v] for k, v in self.cols.items() | |
| } | |
| def to_dict(self): | |
| if not self.prepared: | |
| self.prepare() | |
| return {k.data: [d.data for d in v] for k, v in self.cols.items()} |
No description provided.