Skip to content

[DE-545] UTF-8 names #260

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

Merged
merged 4 commits into from
Jul 28, 2023
Merged

[DE-545] UTF-8 names #260

merged 4 commits into from
Jul 28, 2023

Conversation

apetenchea
Copy link
Member

It is the responsibility of the server to enable extended names. The driver can send any UTF-8 string along with the request.
Therefore, this PR doesn't really add any new and exciting features. Instead, it adds two tests (one for databases and one for collections), making sure the driver can successfully forward UTF-8 names to the server.
Note that, in order to run the tests, the server has to be started with the --database.extended-names option. Unfortunately, this is only possible starting with 3.11 docker images, and we're currently using 3.10.9 for our GitHub actions. Nevertheless, I have added the parameter as a comment for now. I can confirm the tests have passed locally for versions >= 3.11.0.

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2023

Codecov Report

Merging #260 (2c7bcf9) into main (8b09e07) will increase coverage by 0.41%.
The diff coverage is 93.75%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #260      +/-   ##
==========================================
+ Coverage   98.58%   99.00%   +0.41%     
==========================================
  Files          26       26              
  Lines        3899     3931      +32     
==========================================
+ Hits         3844     3892      +48     
+ Misses         55       39      -16     
Impacted Files Coverage Δ
arango/collection.py 98.48% <50.00%> (+2.00%) ⬆️
arango/client.py 98.30% <75.00%> (-1.70%) ⬇️
arango/aql.py 95.04% <100.00%> (+0.04%) ⬆️
arango/cursor.py 100.00% <100.00%> (ø)
arango/http.py 100.00% <100.00%> (ø)

@dothebart
Copy link

please try to create indices on these collections / databases as well.

@aMahanna
Copy link
Member

please try to create indices on these collections / databases as well.

hey @dothebart, happy to take over while Alex is OOO

Collection Index creation has been added in 3dd1fc6, but can you clarify what you mean by Database Index creation?

@dothebart
Copy link

the test should create indices on database/collections with unicode names.

@aMahanna
Copy link
Member

aMahanna commented Jul 24, 2023

Ok based on our internal discussion @dothebart a test has been added to creating an Index within a utf8 ArangoDB Collection which is within a utf8 ArangoDB Database

See 2c102bb

Not sure if this is exactly what you're looking for, but happy to adjust.

Also not sure if this added test should live in test_collection.py or test_database.py, given that it touches on both.

Copy link

@dothebart dothebart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't have any opinion on the location of the test.
LGTM.

@aMahanna aMahanna merged commit 064cd00 into main Jul 28, 2023
@aMahanna aMahanna deleted the feature/de-545-utf-8-names branch July 28, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants