-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add collection information #17
Add collection information #17
Conversation
20842ab
to
b05c690
Compare
b05c690
to
b165b37
Compare
src/collection.rs
Outdated
#[maybe_async] | ||
pub async fn checksum_with_options( | ||
&self, | ||
with_revisions: Option<bool>, |
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.
Why wrap the bool in Option
? Can we use bool only?
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.
Hey, I was thinking it was "optional" parameters, so I used options. But you are right, we could use only bool. I'll make the try with only bool and see how it looks like.
I updated with bool, the code is a bit shorter. Option was useless. It is better now :) |
If you are OK with the structures I made, I will check the last checkbox. Since there is no inheritance in Rust and I am originally a typescript guy, I wasn't sure about the strategy. So I choose to make more structs instead of having a giant struct with many options. I think that having good clear type is important, isn't it ? |
The structures are pretty good! Thank you SO much. |
Cool ! You'r welcome :) Then we are good to go ! |
src/collection.rs
Outdated
) -> Result<CollectionChecksum, ClientError> { | ||
let mut query_parameters = "".to_owned(); | ||
|
||
if with_revisions == true || with_data == true { |
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.
Here what we want is to add query to the requested url. So we may consider using the set_query method in url::Url
.
let mut url = self.base_url.join("checksum").unwrap();
if with_revisions{
url.set_query(Some("withRevisions=true"));
}
if with_data {
url.set_query(Some("withData=true"));
}
// make request and serialize response
...
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.
Hey ! Thank you ! I do not know why I did not use it in the first place.
It seems that set_query resets the query from the beginning, so I had to use url.query_pairs_mut()
instead.
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.
Wow, I didn't know that. Thank you.
Actually with_revisions
and with_data
is bool, so we don't need to explicitly compare them to a bool. That's fine, never mind.
Looks great. Ready to merge? |
Yes :) It is good to go ! |
Well, I guess that we need to implement all the methods we find on Arangodb documentation for collection or maybe not.
Here is the todo list
implement get_collection -> done previously in database::collection
update base_url for collection and add doc to it -> done in previous PR
implement collection::properties -> done in previous PR
add unit test for properties -> done in previous PR
add or update structures that match the information from the doc
Should we add
Option<T>
on CollectionDetails or having new structures?implement document count
add unit test for get count
implement statistics
add unit test for statistics
Should we do something about the shard stuff there and there ?
Question : the response in Arangodb 3.7 and 3.6 are different for revision. What is the best way to handle that ?
add unit test for revision id
implement checksum for collection
add unit test for checksum for collection
Should we add read all collections ?