-
Notifications
You must be signed in to change notification settings - Fork 90
Make key description optional #1067
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
@@ -46,6 +46,15 @@ def test_create_keys_default(client, test_key_info): | |||
assert key.indexes == test_key_info["indexes"] | |||
|
|||
|
|||
def test_create_keys_without_desc(client, test_nondescript_key_info): |
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.
I did confirm it breaks without the change in the non-test code.
@@ -206,6 +206,27 @@ def test_key_info(client): | |||
pass | |||
|
|||
|
|||
@fixture(scope="function") | |||
def test_nondescript_key_info(client): |
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.
I don't think this is really necessary, I didn't just change the test_key_info
since that seemed a bit intrusive.
The reason why test_key_info
would not work is because it uses descriptions to identify the keys that it created, which of course is a problem if the descriptions are None.
I think it would probably be best to delete this function and update the original one to use created_at
, or just use the fact that the most recently created key should be first:
NOTE
API keys are displayed in descending order based on their createdAt date. This means that the most recently created keys appear first.
https://www.meilisearch.com/docs/reference/api/keys#get-all-keys
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.
bors merge
Fixes #1038