arangodb / python-arango

The official ArangoDB Python driver.
https://docs.python-arango.com
MIT License
446 stars 74 forks source link

Make `add_index` public #321

Closed aMahanna closed 5 months ago

aMahanna commented 9 months ago

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 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.

aMahanna commented 9 months ago

hey @apetenchea could I get your thoughts on this (PR description) ^

wondering what the best way to proceed is

aMahanna commented 9 months ago

@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;

  1. Should dict parameters of methods that interact with the ArangoDB API contain keys that are snake_case or camelCase? i.e this would apply to more than just the add_index method

  2. Should non-dict parameters of methods that interact with the ArangoDB API be snake_case or camelCase? e.g requestTimeout vs request_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:

aMahanna commented 8 months ago

@apetenchea

agreed! moving forward with this will give us a chance to get community feedback

codecov-commenter commented 5 months ago

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 ```diff @@ 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.

apetenchea commented 5 months ago

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.