Skip to content
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 basic pagination support for fetching values #10

Merged
merged 2 commits into from
Aug 25, 2015

Conversation

0x1mason
Copy link

There's more I could do, but in many places it would require breaking changes where the lib currently returns XInfo (eg, ListFollowers returns UserInfo, iirc). I think it'd be more appropriate to return a list there like it does elsewhere, though perhaps I misunderstand the intent. However, I didn't want to make a change like that without further discussion.

I don't have a non-windows machine to test this on, but the interop imports for the console app shouldn't cause any problems.

@0x1mason 0x1mason force-pushed the add_pagination branch 4 times, most recently from 39fdc76 to 0601982 Compare August 17, 2015 12:01
@@ -11,7 +11,11 @@ namespace SharpBucket{
/// </summary>
public class SharpBucket{
private Authenticate authenticator;
protected string _baseUrl;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change this in all files or on none. Otherwise it will just get quite inconsistent. Lets not change it for now and maybe do a PR just for that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will do

@MitjaBezensek
Copy link
Owner

Just checked the PR (and also the issue about testing that you created last week). I added a few remarks in the comments and also opened an issue about testing #11.

If you have any suggestions about testing it would be great. Was kinda struggling with what to do since it's just api calls. Will probably add a testing framework (probaly NUnit since I'm most familiar to it) to the project, then we can get rid of the Assert method and the interop imports. Since it's only in the console project it shouldn't cause any problems for people installing from NuGet, not sure about devs using non win OSes that would like to contribute though.

In regards to breaking changes: if you are willing to do the changes that's fine by me. The goal should be to cover all the api calls with pagination so the breaking changes will come sooner or later.

@0x1mason
Copy link
Author

@MitjaBezensek Cool, will back out the bits you suggest and make the other changes as long as you're ok with the breaks. Like you, I have the most experience with NUnit so that would be my first impulse. Definitely better than my homebaked Assert ;p

@0x1mason
Copy link
Author

@MitjaBezensek Ok, so I reset the baseUrl changes. All the functions returning lists should have a max parameter. I also got rid of my assert function and the pinvoke and used the Contract namespace (which I had forgotten about) instead.

@@ -7,8 +7,11 @@ namespace SharpBucket.V2{
/// https://confluence.atlassian.com/display/BITBUCKET/Version+2
/// </summary>
public sealed class SharpBucketV2 : SharpBucket{

internal const string BITBUCKET_URL = "https://bitbucket.org/api/2.0/";
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to expose this to have access in EndPoint.

@MitjaBezensek
Copy link
Owner

Great. I'll have some time during the weekend and I'll merge this and start working on the tests. I will use the tests you wrote (just gonna rewrite them for NUnit) and add a few of the basic ones that target the public part of the API.

@0x1mason
Copy link
Author

The "mirror" account has some large repos that make use of a variety of features. You might find it helpful for testing. https://bitbucket.org/mirror/

@MitjaBezensek
Copy link
Owner

Yeah, I saw you using it for testing and checked it out. Will most likely use one of their repos.

@MitjaBezensek MitjaBezensek merged commit 14878f9 into MitjaBezensek:master Aug 25, 2015
@MitjaBezensek
Copy link
Owner

Just merged this, thanks for the PR 👍

I also just added basic testing logic and refactored your pagination tests into proper tests. Will try to add additional tests in the next days, here's a few things I have planned #13 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants