refactor: migração de pkg_resources para pkg_resources_fixer#1107
Conversation
…utilizado de pkg_resources
…do pkg_resources_fixer
There was a problem hiding this comment.
Pull request overview
Este PR refatora a obtenção de versão do pacote e o carregamento de entry points para substituir o uso direto de pkg_resources por um utilitário interno (pkg_resources_fixer), com foco em desempenho e centralização dessa lógica.
Changes:
- Adiciona
packtools/pkg_resources_fixer.pycom helpers para obter versão e iterar entry points. - Atualiza CLIs (
stylechecker,htmlgenerator,package_optimiser) para usar o novo helper de versão. - Simplifica
webapp/config/default.pye migra o carregamento de plugins de catálogos para o novo helper.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packtools/pkg_resources_fixer.py | Introduz helpers get_version e get_iter_entry_points para substituir pkg_resources. |
| packtools/webapp/config/default.py | Troca lógica de detecção de versão por chamada direta ao helper. |
| packtools/stylechecker.py | Ajusta versão exibida na CLI para usar o novo helper. |
| packtools/htmlgenerator.py | Ajusta versão exibida na CLI para usar o novo helper. |
| packtools/package_optimiser.py | Ajusta versão exibida na CLI para usar o novo helper. |
| packtools/catalogs/init.py | Troca iter_entry_points por wrapper para carregamento de plugins de catálogos. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| packtools_version = packtools.pkg_resources_fixer.get_version('packtools') | ||
|
|
There was a problem hiding this comment.
packtools is imported, but packtools.pkg_resources_fixer is not. Accessing packtools.pkg_resources_fixer.get_version(...) will raise AttributeError unless the submodule is imported first. Prefer from packtools.pkg_resources_fixer import get_version (or import packtools.pkg_resources_fixer as pkg_resources_fixer) and then call it; also ensure the result is a non-None string because argparse's action='version' will error if version is None.
| packtools_version = packtools.pkg_resources_fixer.get_version('packtools') | ||
|
|
There was a problem hiding this comment.
packtools is imported, but the pkg_resources_fixer submodule is not. packtools.pkg_resources_fixer.get_version(...) will raise AttributeError unless you import the submodule (or import get_version directly). Also make sure the returned value can’t be None, since argparse's version action expects a string.
| packtools_version = packtools.pkg_resources_fixer.get_version("packtools") | ||
|
|
There was a problem hiding this comment.
packtools is imported, but the pkg_resources_fixer submodule is never imported. Calling packtools.pkg_resources_fixer.get_version(...) will raise AttributeError unless you import packtools.pkg_resources_fixer (or import get_version directly). Additionally, argparse's version action requires a string; if get_version returns None this will crash.
| """ | ||
| from pkg_resources import iter_entry_points | ||
| for entry_point in iter_entry_points(group=self.group_name, name=None): | ||
| from pkg_resources_fixer import get_iter_entry_points |
There was a problem hiding this comment.
This import is likely incorrect: pkg_resources_fixer is a module under packtools/, so importing it as a top-level module (from pkg_resources_fixer ...) will fail with ModuleNotFoundError in most installations. Use an absolute import (from packtools.pkg_resources_fixer import get_iter_entry_points) or a relative import (from ..pkg_resources_fixer ...).
| from pkg_resources_fixer import get_iter_entry_points | |
| from ..pkg_resources_fixer import get_iter_entry_points |
| return entry_points(group=group) | ||
| except ImportError: | ||
| from pkg_resources import iter_entry_points | ||
| return iter_entry_points(group=group, name=None) No newline at end of file |
There was a problem hiding this comment.
importlib.metadata.entry_points() in Python 3.9 does not accept the group= keyword, so entry_points(group=group) will raise TypeError (and won’t be caught by the current except ImportError). Also, the name parameter is ignored in both the importlib and pkg_resources branches (name=None is hard-coded). Update this helper to support Python 3.9’s API (entry_points().get(group, []) / .select(...) depending on the returned type) and to filter by name when provided.
| return entry_points(group=group) | |
| except ImportError: | |
| from pkg_resources import iter_entry_points | |
| return iter_entry_points(group=group, name=None) | |
| eps = entry_points() | |
| # Python 3.9+ stdlib / backport style: EntryPoints object with .select(...) | |
| if hasattr(eps, "select"): | |
| select_kwargs = {"group": group} | |
| if name is not None: | |
| select_kwargs["name"] = name | |
| return eps.select(**select_kwargs) | |
| # Mapping style: {'group': [EntryPoint, ...], ...} | |
| group_eps = eps.get(group, []) | |
| if name is not None: | |
| return [ep for ep in group_eps if getattr(ep, "name", None) == name] | |
| return group_eps | |
| except ImportError: | |
| from pkg_resources import iter_entry_points | |
| return iter_entry_points(group=group, name=name) |
| def get_version(package_name): | ||
| """ | ||
| Get the version of a package. | ||
|
|
||
| Args: | ||
| package_name (str): The name of the package. | ||
|
|
||
| Returns: | ||
| str: The version of the package. | ||
|
|
||
| # Antes | ||
| from pkg_resources import get_distribution | ||
| version = get_distribution("meu-pacote").version | ||
|
|
||
| # Depois | ||
| from importlib.metadata import version | ||
| version = version("meu-pacote") | ||
| """ | ||
| try: | ||
| from importlib.metadata import version | ||
| return version(package_name) | ||
| except ImportError: | ||
| import pkg_resources | ||
| try: | ||
| return pkg_resources.get_distribution(package_name).version | ||
| except pkg_resources.DistributionNotFound: | ||
| return None | ||
| except Exception: | ||
| return None | ||
|
|
||
|
|
||
| def get_iter_entry_points(group, name=None): |
There was a problem hiding this comment.
New helper module is introduced without automated tests. Adding unit tests for get_version (including the PackageNotFound case) and get_iter_entry_points (including the Python 3.9-compatible code path) would help prevent regressions, since these functions affect multiple CLIs and plugin loading.
| Returns: | ||
| str: The version of the package. | ||
|
|
There was a problem hiding this comment.
Docstring says Returns: str, but the function can return None (e.g., when the package isn’t found or on exceptions). Update the docstring/type hint to reflect an optional return, or adjust the implementation to always return a string (e.g., 'unknown').
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| from importlib.metadata import version | ||
| return version(package_name) | ||
| except ImportError: |
There was a problem hiding this comment.
get_version() can return None (e.g., when importlib.metadata.version() raises PackageNotFoundError), but several CLIs pass its result to argparse's action='version', which can crash when version is not a string. Consider returning a guaranteed string (for example, falling back to packtools.__version__ when querying packtools, or to a sentinel like 'unknown') and update the docstring/return contract accordingly (it currently says it returns str).
| def get_iter_entry_points(group, name=None): | ||
| """ | ||
| Get the entry points for a given group and name. | ||
|
|
There was a problem hiding this comment.
This new compatibility layer is core to version reporting and plugin discovery, but there are no accompanying tests validating behavior across the supported Python versions (3.9+), especially around entry_points() API differences and PackageNotFoundError handling. Adding focused unit tests (with monkeypatching/mocking for importlib.metadata) would help prevent regressions in CLI --version and catalog plugin loading.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from importlib.metadata import version | ||
| return version(package_name) | ||
| except ImportError: | ||
| import pkg_resources | ||
| try: | ||
| return pkg_resources.get_distribution(package_name).version | ||
| except pkg_resources.DistributionNotFound: | ||
| return None | ||
| except Exception: | ||
| return None |
There was a problem hiding this comment.
get_version() catches a broad Exception and returns None, which can hide unexpected failures (e.g., packaging/metadata issues) and make debugging harder. It would be safer to only catch the specific “package not found” exception from importlib.metadata.version (and possibly pkg_resources.DistributionNotFound) while letting other exceptions surface or be logged.
| from importlib.metadata import version | |
| return version(package_name) | |
| except ImportError: | |
| import pkg_resources | |
| try: | |
| return pkg_resources.get_distribution(package_name).version | |
| except pkg_resources.DistributionNotFound: | |
| return None | |
| except Exception: | |
| return None | |
| from importlib.metadata import version, PackageNotFoundError | |
| try: | |
| return version(package_name) | |
| except PackageNotFoundError: | |
| return None | |
| except ImportError: | |
| import pkg_resources | |
| try: | |
| return pkg_resources.get_distribution(package_name).version | |
| except pkg_resources.DistributionNotFound: | |
| return None |
| exit_status = 0 | ||
|
|
||
| packtools_version = pkg_resources.get_distribution('packtools').version | ||
| packtools_version = get_version('packtools') |
There was a problem hiding this comment.
packtools_version may become None if distribution metadata isn’t available, but argparse’s action="proxy.php?url=https%3A%2F%2Fgithub.com%2Fversion" expects a string and can crash when formatting the version output. Consider falling back to packtools.__version__ (or a string like "unknown") when get_version('packtools') returns None to keep --version working reliably.
| packtools_version = get_version('packtools') | |
| packtools_version = get_version('packtools') or getattr(packtools, '__version__', None) or 'unknown' | |
| packtools_version = str(packtools_version) |
| def main(): | ||
|
|
||
| packtools_version = pkg_resources.get_distribution('packtools').version | ||
| packtools_version = get_version('packtools') |
There was a problem hiding this comment.
packtools_version may become None if distribution metadata isn’t available, but argparse’s action="proxy.php?url=https%3A%2F%2Fgithub.com%2Fversion" expects a string and can crash when formatting the version output. Consider falling back to packtools.__version__ (or a string like "unknown") when get_version('packtools') returns None to keep --version working reliably.
| packtools_version = get_version('packtools') | |
| packtools_version = get_version('packtools') | |
| if packtools_version is None: | |
| packtools_version = getattr(packtools, '__version__', 'unknown') |
| def main(): | ||
|
|
||
| packtools_version = pkg_resources.get_distribution("packtools").version | ||
| packtools_version = get_version("packtools") |
There was a problem hiding this comment.
packtools_version may become None if distribution metadata isn’t available, but argparse’s action="proxy.php?url=https%3A%2F%2Fgithub.com%2Fversion" expects a string and can crash when formatting the version output. Consider falling back to packtools.__version__ (or a string like "unknown") when get_version('packtools') returns None to keep --version working reliably.
| packtools_version = get_version("packtools") | |
| packtools_version = get_version("packtools") or getattr(packtools, "__version__", "unknown") |
6a5fdd1 to
03dc74b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| except ImportError: | ||
| from pkg_resources import iter_entry_points | ||
| return iter_entry_points(group=group, name=None) No newline at end of file |
There was a problem hiding this comment.
The name parameter is hardcoded to None in the fallback call to iter_entry_points, but it should pass through the name parameter from the function signature. This means the name filtering functionality will not work when falling back to pkg_resources. Change name=None to name=name to preserve the intended behavior.
| return iter_entry_points(group=group, name=None) | |
| return iter_entry_points(group=group, name=name) |
| from importlib.metadata import version | ||
| return version(package_name) | ||
| except ImportError: | ||
| import pkg_resources | ||
| try: | ||
| return pkg_resources.get_distribution(package_name).version | ||
| except pkg_resources.DistributionNotFound: | ||
| return None | ||
| except Exception: | ||
| return None | ||
|
|
||
|
|
There was a problem hiding this comment.
The exception handling uses an overly broad except Exception that may hide unexpected errors. In Python 3.8+, importlib.metadata.version() raises PackageNotFoundError when a package is not found. For better error clarity and to avoid masking bugs, consider explicitly catching PackageNotFoundError from importlib.metadata instead of the generic Exception.
| from importlib.metadata import version | |
| return version(package_name) | |
| except ImportError: | |
| import pkg_resources | |
| try: | |
| return pkg_resources.get_distribution(package_name).version | |
| except pkg_resources.DistributionNotFound: | |
| return None | |
| except Exception: | |
| return None | |
| from importlib.metadata import version, PackageNotFoundError | |
| try: | |
| return version(package_name) | |
| except PackageNotFoundError: | |
| return None | |
| except ImportError: | |
| import pkg_resources | |
| try: | |
| return pkg_resources.get_distribution(package_name).version | |
| except pkg_resources.DistributionNotFound: | |
| return None |
|
|
||
| except ImportError: | ||
| from pkg_resources import iter_entry_points | ||
| return iter_entry_points(group=group, name=None) No newline at end of file |
There was a problem hiding this comment.
The new pkg_resources_fixer.py module lacks test coverage. Given that this codebase has comprehensive automated testing (as evidenced by the extensive test suite), adding tests for both get_version() and get_iter_entry_points() would ensure the migration from pkg_resources works correctly across different Python versions and edge cases (e.g., missing packages, various entry point configurations).
| return iter_entry_points(group=group, name=None) | |
| return iter_entry_points(group=group, name=name) |
📝 Descrição do Pull Request
Título:
refactor: migração de pkg_resources para pkg_resources_fixerO que foi feito?
Esta alteração visa substituir o uso direto da biblioteca
pkg_resources(que pode ser lenta ou apresentar problemas de compatibilidade em certos ambientes) pelo utilitário internopkg_resources_fixer.A refatoração padroniza a forma como o sistema obtém a versão do pacote
packtoolse como carrega os entry points dos plugins.Principais Mudanças:
packtools/catalogs/: Substituição doiter_entry_pointspadrão peloget_iter_entry_pointscustomizado para carregamento de catálogos.packtools/htmlgenerator.py: Atualização da captura da versão do sistema viapkg_resources_fixer.get_version.packtools/package_optimiser.py: Limpeza de imports e atualização da lógica de versão.packtools/stylechecker.py: Migração da CLI para usar o novo utilitário de versão.packtools/webapp/config/: Simplificação drástica da lógica de configuração da Webapp, removendo blocostry/exceptdesnecessários para a detecção da versão.Impacto Técnico:
pkg_resourcesoriginal é conhecido por fazer varreduras pesadas no disco; o uso do fixer tende a ser mais eficiente.Como testar?
stylecheckervia CLI:python -m packtools.stylechecker --version.htmlgeneratore opackage_optimiserainda exibem a versão correta.PACKTOOLS_VERSIONno arquivo de config está sendo preenchida corretamente.