-
Notifications
You must be signed in to change notification settings - Fork 31
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
Support for indexes api #350
Support for indexes api #350
Conversation
@DiscoPYF Hey, we at ArangoDB have a strong interest in supporting this particular driver with our contributions, so PTAL at our first PR;) |
Added support for ArangoDB's Index API: |
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.
@DiscoPYF I did notice some minor code style inconsistencies - mostly some extra whitespace, but also we usually enforce using braces for if statements, even if they are oneliners - but I have not picked these out in the PR because I think we can apply these standards automatically using a .editorconfig file: https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/code-style-rule-options?view=vs-2022. So I'm going to make a new issue to propose we introduce the .editorconfig file based on the defaults published at that link, and then apply those standards automatically to all files in the project.
@tjoubert The changes look great - consistent with the existing codebase, and with tests included. Thanks for the contribution! Since this is a big PR, I'm approving but will ask @DiscoPYF to also give the code a look over.
Created #351 relating to fixing/homogenising styles for all existing files. |
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.
@tjoubert Thank you very much for this great contribution! 🥳 We really appreciate the help to get more of the APIs implemented.
Code looks good. I left some minor comments. One of them is about documenting some of the properties and behaviors that aren't obvious as a caller of the API methods. This is to try minimize the amount of back and forth between the code and doc for developers.
I think we should change the exception types thrown for validation into ArgumentException
before merging. I'm happy to discuss on any of the comments.
arangodb-net-standard/IndexApi/Models/GetAllCollectionIndexesResponse.cs
Show resolved
Hide resolved
@DiscoPYF, @rossmills99 thanks a lot for all the good feedback. I have reviewed everything and fixed the code where needed. I hope we can move forward with merging the PR :-) |
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.
Thank you for the update @tjoubert , looks good to me 👍
} | ||
break; | ||
default: | ||
throw new System.Exception("Invalid index type"); |
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.
This should also be a System.ArgumentException
because indexType
is provided as a method argument. It's not possible to encounter this case in normal circumstances though, so not a big deal.
* POST: /_api/explain: Explain an AQL query POST: /_api/query: Parse an AQL query DELETE: /_api/query/slow: Clears the list of slow AQL queries GET: /_api/query/slow: Returns the list of slow AQL queries DELETE: /_api/query/{query-id}: Kills a running AQL query * Fixed GetSlowAqlQueriesResponse * Fixed * Fixed GetSlowAqlQueriesAsync_ShouldSucceed * Fixed PostParseAqlQueryResponse.BindVars * Support for indexes api #350 * Update for Support for indexes api #350 * Updated for PR #350 and #352 * Added AQL Query Cache Support DELETE: /_api/query-cache: Clears any results in the AQL query cache GET: /_api/query-cache/entries: Returns the currently cached query results GET: /_api/query-cache/properties: Returns the global properties for the AQL query cache PUT: /_api/query-cache/properties: Globally adjusts the AQL query result cache properties * AQL Query Tracking Support * Added RunningAqlQuery.cs * Fixed Tests * Fixed GetCurrentlyRunningAqlQueriesAsync_ShouldSucceed * Fixes after review * Fixed tests
Implemented support for ArangoDB Indexes API