Passing classifying vars and keys on as parameters. #188
Conversation
WalkthroughThe Changes
Poem
✨ 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 (
|
![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
🧹 Nitpick comments (1)
src/pystatis/table.py (1)
151-162: LGTM! Consider reducing repetition.The parameter processing logic is correct and will properly add the classifying variables and keys to the API request when provided. The conditional checks ensure only non-empty values are included.
Consider refactoring to reduce repetition and improve maintainability:
- if classifyingvariable1: - params['classifyingvariable1'] = classifyingvariable1 - if classifyingkey1: - params['classifyingkey1'] = classifyingkey1 - if classifyingvariable2: - params['classifyingvariable2'] = classifyingvariable2 - if classifyingkey2: - params['classifyingkey2'] = classifyingkey2 - if classifyingvariable3: - params['classifyingvariable3'] = classifyingvariable3 - if classifyingkey3: - params['classifyingkey3'] = classifyingkey3 + # Add classifying variables and keys if provided + classifying_params = { + 'classifyingvariable1': classifyingvariable1, + 'classifyingkey1': classifyingkey1, + 'classifyingvariable2': classifyingvariable2, + 'classifyingkey2': classifyingkey2, + 'classifyingvariable3': classifyingvariable3, + 'classifyingkey3': classifyingkey3, + } + params.update({k: v for k, v in classifying_params.items() if v})
| classifyingvariable1: str = "", | ||
| classifyingkey1: str = "", | ||
| classifyingvariable2: str = "", | ||
| classifyingkey2: str = "", | ||
| classifyingvariable3: str = "", | ||
| classifyingkey3: str = "", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for the new parameters.
The new classifying variable and key parameters are not documented in the method's docstring. Given the comprehensive documentation for all other parameters, these should be documented as well to maintain consistency and help users understand their purpose.
Add documentation for the new parameters in the docstring. Also consider refactoring to use a more structured approach for the classifying pairs to improve maintainability:
+ classifyingvariable1 (str, optional): Code of the first classifying variable for data filtering.
+ classifyingkey1 (str, optional): Key value(s) for the first classifying variable.
+ Multiple values can be passed as a comma-separated list.
+ classifyingvariable2 (str, optional): Code of the second classifying variable for data filtering.
+ classifyingkey2 (str, optional): Key value(s) for the second classifying variable.
+ Multiple values can be passed as a comma-separated list.
+ classifyingvariable3 (str, optional): Code of the third classifying variable for data filtering.
+ classifyingkey3 (str, optional): Key value(s) for the third classifying variable.
+ Multiple values can be passed as a comma-separated list.Alternatively, consider accepting a more structured parameter like classifying_pairs: list[tuple[str, str]] = None for better maintainability.
📝 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.
| classifyingvariable1: str = "", | |
| classifyingkey1: str = "", | |
| classifyingvariable2: str = "", | |
| classifyingkey2: str = "", | |
| classifyingvariable3: str = "", | |
| classifyingkey3: str = "", | |
| def get_data( | |
| self, | |
| series: str, | |
| start_time: datetime | date | str, | |
| end_time: datetime | date | str, | |
| include_observations: bool = False, | |
| classifyingvariable1: str = "", | |
| classifyingkey1: str = "", | |
| classifyingvariable2: str = "", | |
| classifyingkey2: str = "", | |
| classifyingvariable3: str = "", | |
| classifyingkey3: str = "", | |
| ) -> DataFrame: | |
| """ | |
| Fetch data for the given series between start and end times. | |
| Parameters: | |
| series (str): Code of the data series. | |
| start_time (datetime|date|str): Start of the time range. | |
| end_time (datetime|date|str): End of the time range. | |
| include_observations (bool, optional): Whether to include observations. | |
| classifyingvariable1 (str, optional): Code of the first classifying variable for data filtering. | |
| classifyingkey1 (str, optional): Key value(s) for the first classifying variable. | |
| Multiple values can be passed as a comma-separated list. | |
| classifyingvariable2 (str, optional): Code of the second classifying variable for data filtering. | |
| classifyingkey2 (str, optional): Key value(s) for the second classifying variable. | |
| Multiple values can be passed as a comma-separated list. | |
| classifyingvariable3 (str, optional): Code of the third classifying variable for data filtering. | |
| classifyingkey3 (str, optional): Key value(s) for the third classifying variable. | |
| Multiple values can be passed as a comma-separated list. | |
| Returns: | |
| DataFrame: The requested time series data. | |
| """ | |
| # method body follows… |
🤖 Prompt for AI Agents
In src/pystatis/table.py around lines 44 to 49, the new parameters
classifyingvariable1, classifyingkey1, classifyingvariable2, classifyingkey2,
classifyingvariable3, and classifyingkey3 lack documentation in the method's
docstring. Add clear descriptions for each of these parameters in the docstring
to maintain consistency and clarify their purpose. Additionally, consider
refactoring the method signature to replace these multiple parameters with a
single structured parameter like classifying_pairs: list[tuple[str, str]] = None
to improve code maintainability and readability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #188 +/- ##
==========================================
- Coverage 80.89% 80.26% -0.63%
==========================================
Files 12 12
Lines 581 593 +12
==========================================
+ Hits 470 476 +6
- Misses 111 117 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -142,6 +148,19 @@ def get_data( | |||
| "format": "ffcsv", | |||
| } | |||
|
|
|||
| if classifyingvariable1: | |||
There was a problem hiding this comment.
you actually don't need to check the variable here, it doesn't hurt to add empty variables this will be just ignored by the API
| @@ -142,6 +148,19 @@ def get_data( | |||
| "format": "ffcsv", | |||
There was a problem hiding this comment.
you can directly add the parameters here
|
to pass the ci you need to run |
Passing the classifying variables and keys as arguments to the
Table.get_data()function no longer works. The changes in this PR add this option back.Here is an example:
This used to work in previous versions of pystatis, it stopped working at some point, and this PR tries to bring back that functionality.
This is related to issue #157.
Summary by CodeRabbit