harshith1118 commented on code in PR #1402:
URL: https://github.com/apache/hamilton/pull/1402#discussion_r2404095215
##########
hamilton/cli/logic.py:
##########
@@ -337,3 +338,43 @@ def _read_py_context(file_path: Path) -> dict:
context[k] = getattr(module, k, None)
return context
+
+
+def _read_toml_context(file_path: Path) -> dict:
+ """Read context from a TOML file. For pyproject.toml, looks for Hamilton
configuration in [tool.hamilton] section."""
+ try:
+ import tomli # Using tomli for compatibility with older Python
versions
Review Comment:
Yes, it should be
added as an optional dependency. Here's why:
1. Current Implementation: The code already handles the case where tomli
is not installed by raising a
helpful ImportError with clear instructions to install it.
2. User Experience: Having TOML support available as an optional extra
(like pip install sf-hamilton[cli] or
pip install sf-hamilton[toml]) would provide a better user experience
than requiring manual installation
of tomli.
3. Consistency: The CLI already has optional dependencies (like for
visualization), so adding TOML support
as an optional dependency fits the existing pattern.
4. Backwards Compatibility: Making it optional ensures that users who
don't need TOML support don't have to
install additional dependencies.
The dependency should likely be added to the pyproject.toml file under
optional dependencies for the CLI
feature, so users can install it as pip install sf-hamilton[cli] which
would include all necessary
dependencies for CLI functionality including TOML support.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
