-
Notifications
You must be signed in to change notification settings - Fork 1.4k
enabled paging support for fb service requests #533
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
Conversation
Hi @cbarkerms, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
regarding breaking changes: Is there a way to keep the previous signatures and add new methods which return the incremental collections |
I considered that, but it was going to result in bloat and the breaking changes are minor (and in many cases non-existent) IMO. |
I would love to get more thoughts here. You're still returning an IList. So this could be enough. |
public async Task<List<FacebookPost>> RequestAsync(FacebookDataConfig config, int maxRecords = 20) | ||
{ | ||
return RequestAsync<FacebookPost>(config, maxRecords); | ||
return await RequestAsync<FacebookPost>(config, maxRecords, FacebookPost.Fields); |
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.
I think that async/await is not required
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.
agreed not need to await.. just return the task and let the caller await
"Microsoft.NETCore.UniversalWindowsPlatform": "5.1.0", | ||
"StyleCop.Analyzers": "1.0.0", | ||
"winsdkfb": "0.11.20160720.1" | ||
"winsdkfb": "0.12.20161020.4" |
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.
we need to update the nuspec as well
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.
done
docs/services/Facebook.md
Outdated
|
||
// Get current user's photo albums | ||
await FacebookService.Instance.GetUserAlbumsAsync(); | ||
await FacebookService.Instance.GetUserAlbumsAsync(20, FacebookAlbum.Fields); |
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 adding Fields here?
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.
we have a method with original signature.. we should continue using that.. no need to change that here
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.
Either GetUserAlbumsAsync() or GetUserAlbumsAsync(20) result in an ambiguous call:
The call is ambiguous between the following methods or properties: 'FacebookService.GetUserAlbumsAsync(int, string)' and 'FacebookService.GetUserAlbumsAsync(int, string, int)'
Here are the two overloads:
- public async Task<List> GetUserAlbumsAsync(int maxRecords = 20, string fields = null)
- public async Task<IncrementalLoadingCollection<FacebookRequestSource, FacebookAlbum>> GetUserAlbumsAsync(int pageSize = 20, string fields = null, int maxPages = 5)
#1 is the original, unchanged signature. I could remove optionals off of #2 to resolve? New signature would be:
public async Task<IncrementalLoadingCollection<FacebookRequestSource, FacebookAlbum>> GetUserAlbumsAsync(int pageSize, int maxPages, string fields = null)
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.
I think this makes sense as you will call the second one for paging support
|
||
// Get current user's photo albums | ||
await FacebookService.Instance.GetUserAlbumsAsync(); | ||
await FacebookService.Instance.GetUserAlbumsAsync(20, FacebookAlbum.Fields); |
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.
methods with original signatures have been added.. should we not leave them as is ? or are we marking those obsolete and suggesting use of new ones ?
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.
please see above - we can potentially avoid deprecating...
Some headers are missing (the CI failed) and you need to sign the CLA from DNF :) |
@cbarkerms, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
#366
The service calls to Facebook are now paged - defaulting to a page size 20 and a maximum of 5 pages, which can be easily overridden.
Key changes -
These generally will not require changes if you are assigning directly to an ItemsSource, or letting the compiler figure the type out with a 'var'. The SampleApp for example did not require any changes.