Hua work miscs#15
Conversation
…alias() Previously the class was used directly with getattr(), which only finds class-level attributes. Now the class is instantiated first so that self.OPTS and self.ALIAS set in __init__ are accessible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates how PgRST.load_opts_alias() locates OPTS/ALIAS for per-document modules and bumps the package version, plus attempts to include RST template files in the distribution.
Changes:
- Change
load_opts_alias()resolution order to prefer a module-defined class before module-levelOPTS/ALIAS. - Bump project version to
2.0.4. - Add a MANIFEST rule intended to include RST template files in package artifacts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/rda_python_miscs/pg_rst.py |
Changes logic/docs for resolving OPTS/ALIAS from imported doc modules. |
pyproject.toml |
Version bump from 2.0.3 to 2.0.4. |
MANIFEST.in |
Adds an include rule for template files (currently points at a non-existent directory name). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (obj for _, obj in inspect.getmembers(mod, inspect.isclass) | ||
| if obj.__module__ == modname), |
There was a problem hiding this comment.
inspect.getmembers(mod, inspect.isclass) returns members sorted by name, not in source-definition order, so this doesn't actually select the “first class defined in that module” as the docstring states. If definition order matters, iterate over mod.__dict__ (insertion-ordered) and filter classes from there, or adjust the wording to match the actual behavior.
| (obj for _, obj in inspect.getmembers(mod, inspect.isclass) | |
| if obj.__module__ == modname), | |
| ( | |
| obj | |
| for obj in mod.__dict__.values() | |
| if inspect.isclass(obj) and obj.__module__ == modname | |
| ), |
| obj = cls() | ||
| opts = getattr(obj, 'OPTS', None) | ||
| alias = getattr(obj, 'ALIAS', None) |
There was a problem hiding this comment.
This instantiates the first class found via cls(). That can raise TypeError if the class requires constructor arguments, and it can trigger side effects in __init__. The docstring also says OPTS/ALIAS are class-level attributes, so it should read from the class (getattr(cls, ...)) rather than an instance.
| obj = cls() | |
| opts = getattr(obj, 'OPTS', None) | |
| alias = getattr(obj, 'ALIAS', None) | |
| # Read class-level attributes directly from the class to avoid | |
| # instantiating it (which may require constructor arguments or | |
| # trigger side effects in __init__). | |
| opts = getattr(cls, 'OPTS', None) | |
| alias = getattr(cls, 'ALIAS', None) |
| if cls is not None: | ||
| obj = cls() | ||
| opts = getattr(obj, 'OPTS', None) | ||
| alias = getattr(obj, 'ALIAS', None) | ||
| else: | ||
| # 2. Fall back to module-level attributes when no class is found. | ||
| opts = getattr(mod, 'OPTS', None) | ||
| alias = getattr(mod, 'ALIAS', None) |
There was a problem hiding this comment.
If a module defines at least one class but that first class doesn't have OPTS, this code will not fall back to module-level OPTS even though the docstring describes module-level as a fallback when no qualifying class is found. Consider continuing to search for a class that defines OPTS, or falling back to module-level when opts is still None after checking the class.
| @@ -1 +1,2 @@ | |||
| include src/rda_python_miscs/*.usg | |||
| include src/rda_python_miscs/rst_templetes/*.temp | |||
There was a problem hiding this comment.
The directory name here appears to be misspelled: the repo contains src/rda_python_miscs/rst_templates/, not rst_templetes/. As written, the template files won’t be included in the sdist/wheel; update the path to the correct directory name.
| include src/rda_python_miscs/rst_templetes/*.temp | |
| include src/rda_python_miscs/rst_templates/*.temp |
No description provided.