Feature/collection-properties

[arn-the-long-beard]

Hey,
First pull request ever on Github. Every feedback are welcomed !!!
Here is what I going to do :

• update base_url for collection and add doc to it
• implement collection::properties
• add unit test for properties

Is feature/properties good or feature/collection-properties better maybe for my local branch name?

Also I have some commit on readme, I guess it is because I had derived my branch from master in the first place and dev does not have the commit. Is it dirty to have it there ?

I tested the code and it works good. I will continue my work on the collection.rs. Maybe I should improve the unit test also and check for the properties instead of just no error.

[arn-the-long-beard]

Wait, there is something wrong in my code. I made a changed yesterday in the code and it broke. I ll fix it.

[arn-the-long-beard]

Okay, I fixed the issue. Ready for comments to improve :)

[fMeow]

There are many unnecessary style changes. Please use cargo fmt to format code by running cargo fmt --all.

[arn-the-long-beard]

Done

I got some messages

Warning: can't set wrap_comments = true, unstable features are only available in nightly channel. multiple time

Should I use nightly build ?

[fMeow]

Thanks.

No need to use nigthly toolchain. Just ignore the warning.

[fMeow]

The E2E test seems too simple. Would you mind examine some field of CollectionDetails that is obtained via Collection::properties(&self)? For instance, name, cache_enabled and etc.

[fMeow]

Hey,
First pull request ever on Github. Every feedback are welcomed !!!
Is feature/properties good or feature/collection-properties better maybe for my local branch name?

Also I have some commit on readme, I guess it is because I had derived my branch from master in the first place and dev does not have the commit. Is it dirty to have it there ?

I tested the code and it works good. I will continue my work on the collection.rs. Maybe I should improve the unit test also and check for the properties instead of just no error.

Local branch name does not matter. Feel free to name anything you think meaningful.

It's should be avoid to include others commit. My bad, I will rebase the develop branch to master. Now you may like to rebase your local branch to origin/develop.

So there will be more implemention on collection, right? If so, maybe you can list some features you are going to implement, so I can have a better understanding of your progress? For example:

• properties
• another feature

It's a good practice but not a necessary one. You can also just keep adding features and tell me in comment once you think it good to merge.

[arn-the-long-beard]

Hey :)
Thank you for all your feedbacks !
Yes I am going to add more features to collection.

So there will be more implemention on collection, right? If so, maybe you can list some features you are going to implement, so I can have a better understanding of your progress?

1 - Yes !! I want to do this ! Where do you want me to put the list ?
2 - Do you know why when my test are failing they do not display correctly the line like in the rust book ?

failures:

---- test_get_properties stdout ----
thread 'test_get_properties' panicked at 'assertion failed: `(left == right)`

Diff < left / right > :
<false
>true

', tests/collection.rs:1:1

I always get 1:1, so it is difficult to know which assert went wrong.

[arn-the-long-beard]

Okay, I made the rebase worked. But I got some duplicated commits. I think it is because I push a second time the same commits.
I made a new branch to replace this one to have stuff clean and I made it from the origin/develop from here which was marked as upstream.

But I still get the 5 previous commits when I did pushed to my forked repo.

[fMeow]

1. You can just list planning features in this PR, in your first comment. You can edit the first comment whenever you make a progress.

2. Maybe it's caused by tokio::test or async_std::test, but I'm not sure.

2 - Do you know why when my test are failing they do not display correctly the line like in the rust book ?

If you use the default feature gates, arangors should be in async code. Note that async unit tests are quite different from a ordinary one, like in the rust book. The unit test should also be spawned in an async context to get async code executed correctly, that is why we need tokio::test for tokio runtime and async_std::test for async-std runtime.

You can try compiling arangors in blocking code to see what happen. If it show the right line that cause assertion to fail, then it is tokio::test that should be blamed.

[fMeow]

Well... I should not force push to develop branch. It seem a bit messy now. My bad.

Maybe you could rollback/checkout to the commit where duplicate commits start?

[arn-the-long-beard]

Thank you for your explanation regarding the tests. I ll try out.

I did rollback and also updated the commit messages to match the standard :). Now it does look like cleaner. I am happy to get through the process on how to contribute :)

[arn-the-long-beard]

I updated the first comment with 3 checkboxes.

Now I am thinking making a new branch where I will actually add all the information from there ( excluding properties of course)
https://www.arangodb.com/docs/stable/http/collection-getting.html.

I think that making one PR for each of the methods is too much. One PR with all of them and nice checkboxes should be fine since each method is pretty simple.

What do you think ?

PS : should I make my new branch from the one from this PR, right ?

[fMeow]

That's great. Thank you SO much!!! Much appreciate your work. :)

Let's done with this PR for properties and put the rest methods you would like to implement in a single new PR.

Is it good to merge now?

[arn-the-long-beard]

Thank you so much for the time you used to explain me stuff :)

I changed the attribute before the test function to use blocking mode but the line is still 1:1 when test failed. But I found a Github issue maybe related to this : rust-lang/rust#68430

It is good to go for the PR !

[fMeow]

It's still a bit messy. Your PR contains some duplicated commits, and that's my fault. Sorry for your inconvinience.

If I am right, this PR contains 3 valuable commits. So let's reset your branch to origin/develop to start cleanly, and apply the 3 valuable commits by cherry-pick one by one.

Here is a step by step guide:

1. create a new branch to store your current work: git checkout -b tmp. You can use git log to see the sha of your valuable commits. Here is the three valuable commits in my git log:

   commit 83b929fca7a801225c4824c348a4f6f319af44c6
   Author: arn-the-long-beard <[email protected]>
   Date:   Thu Jun 18 14:11:04 2020 +0200
   
       test: added unit test for collection::properties
   
   commit 1fe5e1f1937aac4be308a75fd83f7ec284f164b9
   Author: arn-the-long-beard <[email protected]>
   Date:   Thu Jun 18 14:10:39 2020 +0200
   
       feat: implemented collection::properties
   
   commit d1a3c72dac1c02aa5ada047cea0be9b05d165c5f
   Author: arn-the-long-beard <[email protected]>
   Date:   Thu Jun 18 14:10:18 2020 +0200
   
       feat: updated base_url in collection
   

2. return to feature/properties branch: git checkout feature/properties

3. reset to origin/develop: git reset --hard origin/develop

4. use cherry-pick to apply the 3 commits to the cleaned branch:

   1. git cherry-pick d1a3c7
   2. git cherry-pick 1fe5e1
   3. git cherry-pick 83b929
      The sha here should be correct, and you can just copy and paste.

Again, it's my fault and sorry for your inconvinience. I should never use force-push for branches like develop and master.

[arn-the-long-beard]

Hey, no worries !
Shit happens sometime :)

Okay, I ll do it.

[arn-the-long-beard]

Okay it worked :D I just had to change the reset branch and take it from your develop, not mine. I need to learn how to update my forked version of the repo I guess. The sha were also different for some reason.

Yes !! First PR ever, that is so cool. I 'll come soon with more stuff !

Thank you a lot !