-
Notifications
You must be signed in to change notification settings - Fork 8
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
Added method and tests for revoking access #13
Conversation
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 very much for this contribution! Looks very good, just have one nitpick in tests.
I've added a feature that is available already in the uplink-cli, but it isn't presented here. Actually, I need it for uplink-nodejs, I'll propose PR there in a few days.
When this will be merged we can make a new release for uplink-c so you could use official release for uplink-nodejs
Also, I wasn't able to run tests from the testsuite without some changes, because some of C files don't end with a newline. I decided not to fix it in the current PR. Does anybody have the same behavior?
I didn't have anything like that but if you can please push those changes as a separate PR and we will take a look.
}; | ||
|
||
UplinkSharePrefix prefixes[] = { | ||
{"alpha", ""}, |
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 do we need empty prefix?
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.
When I was writing tests, I referred to this line as an example of sharing access.
So I used the same bucket name and the prefix to be consistent with the rest of the codebase.
As far as I understood there are some issues with prefix and because of it, the empty prefix was set.
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.
Good enough for me
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.
Great! Thank you once again for this patch.
If someone tags a new release, I could bring this extension into uplink.NET soon. |
I will make release today |
Great, I will publish PR for uplink.nodejs soon. |
@ishmelev-sp @TopperDEL sorry it's taking a bit more time to release uplink-c because we want to include latest uplink version with some additional improvements |
No problem, take your time! I just need a tagged version to integrate new features and I simply wanted to be "up-to-date" with uplink.NET. Otherwise that features is not necessary for me currently. :) |
I've added a feature that is available already in the uplink-cli, but it isn't presented here. Actually, I need it for uplink-nodejs, I'll propose PR there in a few days.
Also, I wasn't able to run tests from the testsuite without some changes, because some of C files don't end with a newline. I decided not to fix it in the current PR. Does anybody have the same behavior?