-
Notifications
You must be signed in to change notification settings - Fork 111
[CLIENT-3805] Add missing app_id attribute to Cluster class for extended metrics #864
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #864 +/- ##
==========================================
- Coverage 83.38% 83.37% -0.01%
==========================================
Files 99 99
Lines 14394 14406 +12
==========================================
+ Hits 12002 12011 +9
- Misses 2392 2395 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…control over app_id
…valid cluster_name. Fix type in docs
justinlee-aerospike
left a 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.
Changes are okay
Justin already reviewed and approved
There is missing code coverage for returning None from the Cluster class's app_id attribute, but that is fine because it is hard to reproduce that behavior and it's documented that as_cluster.app_id can be NULL in the C client
Extra changes:
app_idto be set toNone.app_idandcluster_names' types to beOptional[str]since they can be set toNone.app_idandcluster_nameoption toNone.cluster_name.Docs
https://aerospike-python-client--864.org.readthedocs.build/en/864/aerospike.html#:~:text=meaning%20‘never%20compress’-,cluster_name,-(Optional[str
https://aerospike-python-client--864.org.readthedocs.build/en/864/aerospike_helpers.metrics.html#aerospike_helpers.metrics.Cluster.app_id
TODO
Manual testing