python-arango icon indicating copy to clipboard operation
python-arango copied to clipboard

wip: make `add_index` public

Open aMahanna opened this issue 1 year ago • 3 comments

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 data parameter of add_index should expect camelCase or snake_case fields

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

aMahanna avatar Feb 02 '24 21:02 aMahanna

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

wondering what the best way to proceed is

aMahanna avatar Feb 08 '24 00:02 aMahanna

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

  • If the answer is camelCase to both 1 and 2, then that would imply a much larger PR

  • If the answer is camelCase for 1 but not 2, then another PR would also be required to address other dict-based parameters

  • However if the answer is camelCase for just the add_index method, then we would be introducing some (albeit small) UX inconsistency in the driver

  • The alternative would be to continue maintaining the snake_case to camelCase to snake_case logic

aMahanna avatar Feb 08 '24 16:02 aMahanna

@apetenchea

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

aMahanna avatar Feb 16 '24 14:02 aMahanna

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.

codecov-commenter avatar May 25 '24 18:05 codecov-commenter

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.

apetenchea avatar May 25 '24 18:05 apetenchea