-
Notifications
You must be signed in to change notification settings - Fork 4
Passing classifying vars and keys on as parameters. #188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Passing classifying vars and keys on as parameters. #188
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe Changes
Poem
✨ Finishing touches🧪 Generate unit tests
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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})
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a small correction, and pls add the new parameters to the docstring Args section so they are properly documented
src/pystatis/table.py
Outdated
"format": "ffcsv", | ||
} | ||
|
||
if classifyingvariable1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/pystatis/table.py
Outdated
"stand": stand, | ||
"language": language, | ||
"quality": quality, | ||
"format": "ffcsv", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can directly add the parameters here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
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