wip: make `add_index` public
CI is expected to fail for now
This PR proposes to switch _add_index to add_index, and to deprecate the add_*_index methods in favour of generalizing to add_index (similar to how we have create_view, create_analayzer, etc.)
However, we need to decide if:
- The
dataparameter ofadd_indexshould expectcamelCaseorsnake_casefields
The current add_*_index methods accept snake_case, convert to camelCase for API purposes, then output snake_case. We need to decide if we want to follow suit for add_index, or if we want to drop this whole snake_case to camelCase to snake_case conversion.
Until then, CI will fail
hey @apetenchea could I get your thoughts on this (PR description) ^
wondering what the best way to proceed is
@apetenchea
I can get behind this change, but it raises the question of what should we do with formatter.py, as that is what is currently driving the snake_case to camelCase conversion logic.
The way I see it, we have two "snake_case vs camelCase" topics;
-
Should
dictparameters of methods that interact with the ArangoDB API contain keys that aresnake_caseorcamelCase? i.e this would apply to more than just theadd_indexmethod -
Should non-
dictparameters of methods that interact with the ArangoDB API besnake_caseorcamelCase? e.grequestTimeoutvsrequest_timeout
I apologize for widening the scope, but it's a thought that just occurred to me now after reading your proposal
Happy to move this convo somewhere else if preferred
My takeaway is:
-
If the answer is
camelCaseto both 1 and 2, then that would imply a much larger PR -
If the answer is
camelCasefor 1 but not 2, then another PR would also be required to address otherdict-based parameters -
However if the answer is
camelCasefor just theadd_indexmethod, then we would be introducing some (albeit small) UX inconsistency in the driver -
The alternative would be to continue maintaining the
snake_casetocamelCasetosnake_caselogic
@apetenchea
agreed! moving forward with this will give us a chance to get community feedback
Codecov Report
Attention: Patch coverage is 12.50000% with 21 lines in your changes are missing coverage. Please review.
Project coverage is 95.95%. Comparing base (
a26e06a) to head (8c1ef8e).
| Files | Patch % | Lines |
|---|---|---|
| arango/collection.py | 12.50% | 21 Missing :warning: |
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@
## main #321 +/- ##
==========================================
- Coverage 98.22% 95.95% -2.28%
==========================================
Files 26 26
Lines 4283 4277 -6
==========================================
- Hits 4207 4104 -103
- Misses 76 173 +97
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
The add_index takes a data parameter - the keys are pure ArangoDB REST API arguments, no snake_case conversion anymore. However, for the output, the legacy methods remain the same, while the new add_index method by default skips formatting.
I have added a generous note in the documentation explaining that. Hopefully, there should be no confusion among the users, as they switch to the add_index method.