feat: Add the metadata interface to the reporter class #6
Conversation
📝 WalkthroughWalkthroughThe constructor of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ReporterBase
Caller->>ReporterBase: __init__(..., metadata)
alt metadata is None or empty
ReporterBase->>ReporterBase: self.metadata = {}
else metadata provided
ReporterBase->>ReporterBase: validate_flat_dict(metadata)
alt validation fails
ReporterBase->>Caller: raise TypeError
else validation succeeds
ReporterBase->>ReporterBase: self.metadata = metadata
end
end
Possibly related issues
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
LKress
changed the title
LKress
changed the title
| @@ -27,6 +28,7 @@ def __init__( | |||
| jobs: List[JobRecordInterface], | |||
| settings: ReportSettingsBase, | |||
| workflow_description: str, | |||
| metadata: MetadataRecordInterface, | |||
There was a problem hiding this comment.
Turn this into a keyword arg with a default of None that is then initialized as an empty dict.
There was a problem hiding this comment.
Reason: this way, we don't need a breaking change in the interface.
|
|
||
| @property | ||
| @abstractmethod | ||
| def parse_yte(self) -> dict: ... |
There was a problem hiding this comment.
Why does the plugin need to be able to parse this? I would just pass a dict with the metadata to the Reporter class below.
There was a problem hiding this comment.
Then, there is not even an interface needed for the metadata record.
There was a problem hiding this comment.
Makes sense, I removed the MetadataRecordInterface and adapted the snakemake code in pull request #3452
| @@ -28,6 +28,7 @@ def __init__( | |||
| settings: ReportSettingsBase, | |||
| workflow_description: str, | |||
| dag: DAGReportInterface, | |||
| metadata: Dict[Any, Any] = None, | |||
There was a problem hiding this comment.
What can we assume about the dict? Is is always single level? Are the keys always strings and the values str or int? Probably yes. In that case, the corresponding Snakemake PR should ensure that and throw an error otherwise, and here we should annotate it as Dict[str, Union[str, int, float]].
There was a problem hiding this comment.
@LKress Do you have the chance to consider this? I have also tried to contact you via discord (hopefully I found the correct username). If not, just send me a PM via discord.
There was a problem hiding this comment.
I think lists of the above mentioned types should be possible too. Therefore, the current implementation allows lists of str, int and float too. The snakemake reporter already handles this. I have added an additional test that checks that an error is thrown, if the metadata is nested in the snakemake repo (snakemake/snakemake@b0cae83).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
snakemake_interface_report_plugins/reporter.py (1)
31-33: Fix the mutable default argument issue.The current implementation uses a mutable default argument
{}, which is flagged by static analysis and violates Python best practices. Based on the past review comments, this should useNoneas the default and initialize an empty dict inside the constructor.Apply this diff to fix the mutable default argument:
- metadata: Optional[ - Dict[str, Union[str, int, float, List[str], List[int], List[float]]] - ] = {}, + metadata: Optional[ + Dict[str, Union[str, int, float, List[str], List[int], List[float]]] + ] = None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_interface_report_plugins/reporter.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🪛 Ruff (0.11.9)
snakemake_interface_report_plugins/reporter.py
33-33: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
53-54: ReporterBase.__post_init__ is an empty method in an abstract base class, but has no abstract decorator
(B027)
🪛 GitHub Actions: CI
snakemake_interface_report_plugins/reporter.py
[error] 46-46: flake8: line too long (115 > 88 characters) (E501)
🔇 Additional comments (4)
snakemake_interface_report_plugins/reporter.py (4)
8-8: LGTM: Import statement properly updated.The import statement correctly adds the necessary typing components for the metadata interface implementation.
49-49: LGTM: Metadata assignment is correct.The assignment of the validated metadata to the instance attribute is properly implemented.
60-67: LGTM: Helper function is well-implemented.The
is_valid_flat_valuefunction correctly validates individual values according to the specified type constraints.
69-79: LGTM: Validation function is robust.The
validate_flat_dictfunction properly validates the dictionary structure, checking both key types and value types using the helper function.
| # ensure that metadata is a key value dictionary | ||
| if not validate_flat_dict(metadata): | ||
| raise TypeError( | ||
| "Metadata must be single level dict[str, str | int | float | list[str] | list[int] | list[float]]]" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fix line length and adjust validation for None default.
The validation logic needs to be updated to handle the None default value, and the error message line is too long (115 > 88 characters).
Apply this diff to fix both issues:
- # ensure that metadata is a key value dictionary
- if not validate_flat_dict(metadata):
- raise TypeError(
- "Metadata must be single level dict[str, str | int | float | list[str] | list[int] | list[float]]]"
- )
+ # ensure that metadata is a key value dictionary
+ if metadata is None:
+ metadata = {}
+ if not validate_flat_dict(metadata):
+ raise TypeError(
+ "Metadata must be single level dict[str, str | int | float | "
+ "list[str] | list[int] | list[float]]]"
+ )📝 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.
| # ensure that metadata is a key value dictionary | |
| if not validate_flat_dict(metadata): | |
| raise TypeError( | |
| "Metadata must be single level dict[str, str | int | float | list[str] | list[int] | list[float]]]" | |
| ) | |
| # ensure that metadata is a key value dictionary | |
| if metadata is None: | |
| metadata = {} | |
| if not validate_flat_dict(metadata): | |
| raise TypeError( | |
| "Metadata must be single level dict[str, str | int | float | " | |
| "list[str] | list[int] | list[float]]]" | |
| ) |
🧰 Tools
🪛 GitHub Actions: CI
[error] 46-46: flake8: line too long (115 > 88 characters) (E501)
🤖 Prompt for AI Agents
In snakemake_interface_report_plugins/reporter.py around lines 43 to 48, update
the validation logic to allow for the metadata parameter to be None by adding a
condition to skip validation if metadata is None. Also, split the long error
message string into multiple shorter lines to ensure no line exceeds 88
characters, improving readability and adhering to style guidelines.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
snakemake_interface_report_plugins/reporter.py (2)
31-33: Fix mutable default argument to align with past review feedback.The current implementation uses a mutable default argument, which violates Python best practices and contradicts past review comments requesting a keyword arg with None default.
Apply this diff to fix the mutable default argument:
- metadata: Optional[ - Dict[str, Union[str, int, float, List[str], List[int], List[float]]] - ] = {}, + metadata: Optional[ + Dict[str, Union[str, int, float, List[str], List[int], List[float]]] + ] = None,
43-52: Fix line length violation and handle None default.The validation logic has line length issues and needs to handle the None default case.
Apply this diff to fix both issues:
- # ensure that metadata is a key value dictionary - if not validate_flat_dict(metadata): - raise TypeError( - ( - "Metadata must be single level " - "dict[str, str | int | float | list[str] | list[int] | list[float]]]" - ) - ) - - self.metadata = metadata + # ensure that metadata is a key value dictionary + if metadata is None: + metadata = {} + if not validate_flat_dict(metadata): + raise TypeError( + "Metadata must be single level " + "dict[str, str | int | float | " + "list[str] | list[int] | list[float]]]" + ) + + self.metadata = metadata
🧹 Nitpick comments (1)
snakemake_interface_report_plugins/reporter.py (1)
56-57: Consider making__post_init__abstract if subclasses should implement it.The empty
__post_init__method could be made abstract if subclasses are expected to provide implementations, or add a docstring explaining its purpose as an optional hook.If subclasses should implement this method:
+ @abstractmethod def __post_init__(self): - pass + ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_interface_report_plugins/reporter.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🪛 Ruff (0.11.9)
snakemake_interface_report_plugins/reporter.py
33-33: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
56-57: ReporterBase.__post_init__ is an empty method in an abstract base class, but has no abstract decorator
(B027)
🪛 GitHub Actions: CI
snakemake_interface_report_plugins/reporter.py
[error] 48-48: flake8: line too long (89 > 88 characters) (E501)
🔇 Additional comments (2)
snakemake_interface_report_plugins/reporter.py (2)
8-8: LGTM! Import additions are appropriate.The added typing imports are necessary for the metadata type annotations.
63-82: LGTM! Helper functions are well-implemented.The validation functions correctly enforce the metadata constraints:
is_valid_flat_valueproperly checks for valid types and listsvalidate_flat_dictensures dictionary structure with string keys- Logic is comprehensive and handles edge cases appropriately
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
snakemake_interface_report_plugins/reporter.py (2)
31-33: Fix the mutable default argument issue.The use of
{}as a default argument creates a mutable default, which can lead to shared state issues. This was flagged in previous reviews and by static analysis.Apply this diff to fix the mutable default:
- metadata: Optional[ - Dict[str, Union[str, int, float, List[str], List[int], List[float]]] - ] = {}, + metadata: Optional[ + Dict[str, Union[str, int, float, List[str], List[int], List[float]]] + ] = None,
43-51: Handle None default and improve validation logic.The validation logic doesn't properly handle the
Nonedefault value that should be used instead of the mutable default. This was identified in previous reviews.Apply this diff to fix the None handling:
- # ensure that metadata is a key value dictionary - if not validate_flat_dict(metadata): - raise TypeError( - ( - "Metadata must be single level " - "dict[str, str | int | float | " - "list[str] | list[int] | list[float]]]" - ) - ) + # ensure that metadata is a key value dictionary + if metadata is None: + metadata = {} + if not validate_flat_dict(metadata): + raise TypeError( + ( + "Metadata must be single level " + "dict[str, str | int | float | " + "list[str] | list[int] | list[float]]]" + ) + )
🧹 Nitpick comments (1)
snakemake_interface_report_plugins/reporter.py (1)
57-58: Consider making the empty__post_init__method abstract.The empty
__post_init__method in an abstract base class should be decorated with@abstractmethodif subclasses are expected to implement it, or removed if it's not needed.If subclasses should implement this method, apply this diff:
+ @abstractmethod def __post_init__(self): - pass + ...If this method is not needed, consider removing it entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_interface_report_plugins/reporter.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🪛 Ruff (0.11.9)
snakemake_interface_report_plugins/reporter.py
33-33: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
57-58: ReporterBase.__post_init__ is an empty method in an abstract base class, but has no abstract decorator
(B027)
🔇 Additional comments (4)
snakemake_interface_report_plugins/reporter.py (4)
8-8: LGTM on the import additions.The new typing imports are correctly added to support the enhanced type annotations for the metadata parameter.
53-53: LGTM on the metadata assignment.The assignment is correct and will work properly once the None handling is fixed in the validation logic above.
64-71: LGTM on the value validation helper function.The
is_valid_flat_valuefunction correctly validates individual values and lists of allowed types. The logic properly checks for the supported types (str,int,float) and validates that lists contain only these types.
73-83: LGTM on the dictionary validation helper function.The
validate_flat_dictfunction correctly validates the entire dictionary structure, ensuring all keys are strings and all values pass the flat value validation. The logic is sound and matches the type constraints defined in the constructor.
| # ensure that metadata is a key value dictionary | ||
| if not validate_flat_dict(metadata): | ||
| raise TypeError( | ||
| ( | ||
| "Metadata must be single level " | ||
| "dict[str, str | int | float | " | ||
| "list[str] | list[int] | list[float]]]" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Can you please move this check into the main snakemake, before the Report class is called?
The reason is that the type already asserts that the dictionary is valid. Hence, the validation has to happen before the dict is passed to the metadata argument.
This is required, to allow custom metadata in the report (snakemake/snakemake#3452).
Summary by CodeRabbit