Open core-ai-bot opened 3 years ago
core-ai-bot
commented
3 years ago 
Looks like this didn't get assigned a priority in standup. I'm inclined to call it medium priority, even though it's obviously not a functional issue, because we need to make sure we continue to acknowledge our community contributors. Plus the fix is relatively simple :)
core-ai-bot
commented
3 years ago core-ai-bot
commented
3 years ago 
@njx For acknowledging the work of contributors, are you ok with showing "just" 300 contributors?
As I said above, I hardcoded this limit to prevent the dialog from getting too big and loading too long, but I wonder what's your opinion on that.
core-ai-bot
commented
3 years ago
I think if we're lucky enough to get to more than 300 contributors, we can just increase the limit again :) I'm fine with not creating a more general solution right now.
core-ai-bot
commented
3 years ago
(I believe we were at something like 200 contributors the last time I checked, so I think we have awhile before we run into that problem...)
core-ai-bot
commented
3 years ago
Well, I'm a bit confused now. It looks like the max per_page value GitHub allows is 100 (it could be that this limit was there before, but we just didn't notice it), which is making it much more difficult for us to show all contributors.
Try https://api.github.com/repos/adobe/brackets/contributors?per_page=300&page=2
We've got exactly 198 contributors right now. Not bad.
core-ai-bot
commented
3 years ago
So does that mean that the PR that got merged isn't actually showing all the contributors? If so, then I guess we have to bite the bullet and iterate over all the pages.
If I remember the GitHub API (at least for some other cases I've used), there's actually a link in the returned data from the first page that gives you the proper link to follow to get the next page. So we don't have to hard-code the URLs for future pages.
core-ai-bot
commented
3 years ago
Yes, this means basically that this PR is only showing 100 contributors of 198. I just wrote to the GitHub support asking for the best way, but I don't expect an API change.
And no, there's no link to the next page, but that's not a big problem. Hardcoding shouldn't be a problem.
core-ai-bot
commented
3 years ago
Ah, you're right. But yeah, it's not a big deal to just set the parameter for each page.
core-ai-bot
commented
3 years ago
Ah, I just got an answer from the support. That's what they said:
Sorry this tripped you up. We announced on the blog 1 and via the Developer Program 2 that we'd start enforcing pagination on many resource listings. The max page size is 100 and you'll need to follow pagination links 3 in the response headers to fetch the rest.
There's no way to get those in a single request. We have to enforce pagination to keep the API fast for everyone. I'm sure you understand.
core-ai-bot
commented
3 years ago
Ah, right - the pagination links are in the headers, not in the body of the response:
Link: <https://api.github.com/repositories/2935735/contributors?per_page=100&page=2>; rel="next", <https://api.github.com/repositories/2935735/contributors?per_page=100&page=2>; rel="last"
But there's no reason to use those - we can just use the hard-coded URL and set the page parameter.
core-ai-bot
commented
3 years ago 
We might want it to find the last page and stop requesting data, but the same can be done when the data returned is empty.
core-ai-bot
commented
3 years ago
If you can wait until Sunday, I can put up a PR then. Else, you can try it yourselves.
@TomMalbran I thought about a check if (data.length < 100) lastPage = true, so that the next page wouldn't even be loaded.
core-ai-bot
commented
3 years ago
That make sense too. Notice that in the case of having n*100 contributors the last request will have length 0, but that might not even be a case to consider separately.
core-ai-bot
commented
3 years ago
If we happen to do one extra GET in that case it won't kill us :)
core-ai-bot
commented
3 years ago
That's a very rare case, and there's no other way to handle it (When we are on the second page with now 200 contributors, we can't know how many there are on the third page).
core-ai-bot
commented
3 years ago
The point is not that we would do an extra request. That is fine, but to notice that it will have 0 results. But anyway if we are collecting all the data into one array, that won't even matter.
contributors = [];
page = 1;
lastPage = false;
while (!lastPage) {
data = requestContributors(page);
contributors += data;
page++;
lastPage = data.length < 100;
}
Yup. Or we could incrementally update the display as we go along (with the spinner moving down below the previous "page" of results), which might be slightly nicer, but that's probably gravy.
core-ai-bot
commented
3 years ago
In that case I guess that the second load of contributors could happen after the user scrolls to the bottom of the list. We could show the loader by default, but delay the request.
core-ai-bot
commented
3 years ago
@TomMalbran That would be a very cool solution, but the most effort as well.
Is += working for arrays? I would have used contributors.concat(data), but if your way works, that's even better. Good to know.
core-ai-bot
commented
3 years ago
Actually, it doesn't work. So you need to use concat... Too much PHP lately
core-ai-bot
commented
3 years ago 
Hey, just an FYI, the response headers appear to only include the link to the "next" page if there is one. So by using these you could avoid making that extra request.
core-ai-bot
commented
3 years ago
I'd suggest we should first work on a simple and working implementation, which needn't do more than showing all the contributors, and after that we can work on a nicer behavior.
The positive side effect is that there is no hardcoded limit any more.
core-ai-bot
commented
3 years ago
@TuckerWhitehouse I thought about that as well, but it's really only a 1% chance to be in such a situation...
core-ai-bot
commented
3 years ago
I was curious how hard it would be to implement this using the headers provided by github so I wrote the the following code to be used in place of https://github.com/adobe/brackets/blob/master/src/help/HelpCommandHandlers.js#L86-L111
Because this uses the headers from github, which only includes a "next" if there is a next page, this will never make an unnecessary request.
(function _getContributors(url, contributorsList) {
$.getJSON(url).done(function (contributorsInfo, statusText, jqXHR) {
// Append Paged Contributors
contributorsList = contributorsList.concat(contributorsInfo);
// Get Link Header
var Link = jqXHR.getResponseHeader('Link');
var hasNext = false;
Link.split(',').some(function (item) {
if (item.indexOf('rel="next"') > -1) {
_getContributors(item.slice(item.indexOf('<')+1, item.indexOf('>')), contributorsList);
hasNext = true;
return true;
}
});
// All Pages Loaded
if (!hasNext) {
// Populate the contributors data
var totalContributors = contributorsList.length;
var contributorsCount = 0;
$contributors.html(Mustache.render(ContributorsTemplate, contributorsList));
// This is used to create an opacity transition when each image is loaded
$contributors.find("img").one("load", function () {
$(this).css("opacity", 1);
// Count the contributors loaded and hide the spinner once all are loaded
contributorsCount++;
if (contributorsCount >= totalContributors) {
$spinner.removeClass("spin");
}
}).each(function () {
if (this.complete) {
$(this).trigger("load");
}
});
}
}).fail(function () {
$spinner.removeClass("spin");
$contributors.html(Mustache.render("<p class='dialog-message'>{{ABOUT_TEXT_LINE6}}</p>", Strings));
});
})(brackets.config.contributors_url, []);
This fixes #7614 and #7615.
I hardcoded it to show 300 contributors at max (because the dialog would get pretty inflated with more), but we can simply increase the limit.
MarcelGerber included the following code: https://github.com/adobe/brackets/pull/7618/commits