Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2529 +/- ##
=======================================
Coverage 85.84% 85.84%
=======================================
Files 414 414
Lines 34386 34397 +11
=======================================
+ Hits 29518 29529 +11
Misses 4868 4868
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --format json option to the cdf modules list command, providing a machine-readable output for CI/CD pipelines. The implementation is well-structured, including updates to the CLI application, the underlying command logic, documentation, and a new unit test. My main feedback is to use typed data structures (dataclasses) for the JSON output to align with the repository's style guide, which emphasizes type safety.
| @staticmethod | ||
| def _list_row_from_module(module: BuiltModule) -> dict[str, str | int]: | ||
| return { | ||
| "module_name": module.name, | ||
| "resource_folders": len(module.resources), | ||
| "resources": sum(len(resources) for resources in module.resources.values()), | ||
| "build_warnings": module.warning_count, | ||
| "build_result": module.status, | ||
| "location": module.location.path.as_posix(), | ||
| } |
There was a problem hiding this comment.
Using a dict for the module's JSON representation goes against the repository style guide, which favors typed data structures like dataclasses or Pydantic models for better type safety and maintainability. This also applies to the top-level dictionary created in the list method (lines 725-729).
The style guide states:
- (Line 7) 'Use dataclasses and Pydantic models for complex data structures instead of untyped dictionaries'
- (Line 41) 'Use dataclasses or Pydantic models instead of
dict[str, Any]'
To align with the style guide, I recommend defining dataclasses for the entire JSON output structure. This makes the data contract explicit and safer to use.
For example, you could define these dataclasses (you'll need to add from dataclasses import asdict, dataclass):
@dataclass
class ModuleAsJson:
module_name: str
resource_folders: int
resources: int
build_warnings: int
build_result: str
location: str
@dataclass
class ModuleListAsJson:
environment: str
organization_dir: str
modules: list[ModuleAsJson]With these, you could refactor the list method to build and serialize these typed objects, which would also make the _list_row_from_module helper method unnecessary.
References
- The code uses untyped dictionaries for complex data structures (JSON output), whereas the style guide requires using typed structures like dataclasses or Pydantic models for type safety. (link)
- The code uses
dict[str, str | int]for complex data, which is similar todict[str, Any]. The style guide explicitly states to avoid this and use dataclasses or Pydantic models instead. (link)
There was a problem hiding this comment.
Do you want to include these changes @doctrino? If so we'd also need a .dump() for the dataclasses.
There was a problem hiding this comment.
No that is overkill and serves no purpose as far as I see.
There was a problem hiding this comment.
Great change. As this is a new feature, it needs to go behind a feature flag. See https://github.com/cognitedata/toolkit/blob/main/cognite_toolkit/_cdf_tk/feature_flags.py
for how feature flags are used.
3ae1ed4 to
f8f97be
Compare
f8f97be to
88fef5f
Compare
…s list
Description
Adds an
--format jsonoption tocdf modules listto output JSON instead of a Rich table in order to make module list parseable in a reliable manner. We usecdf modules listin order to parse which modules change and have modular deployment in Github. Parsing the Rich table is error-prone, especially in CI/CD where the text can become truncated.Open for alternative solutions to this problem!
Usage:
cdf modules list --format json.Bump
Changelog
Improved
--formatflag tocdf modules listto allow for machine-readable output. Available options:table(default) andjson.