list of active repositories below the "Repository Feature Usage" graph #119
Conversation
|
You mention the top 50 truncation in the pull request body ... but this is not implemented yet, right? |
|
Correct! The top 50 truncation is not implemented yet. Currently the entire list is displayed sorted by most number of pushes. |
|
@larsxschneider I reworked the PR and I now filter the table to display only the top 50 most active repos. |
The number of repos can be impractical to display on a single page: only display the top 50. Add link to table of all active repos.
docs/assets/js/charts.js
Outdated
| @@ -554,10 +554,14 @@ function createTable(table) | |||
| return d; | |||
| }); | |||
|
|
|||
| let displayed = data; | |||
| if (hasConfig($(table), 'slice')) | |||
| displayed = data.slice(readConfig($(table), 'slice')[0], readConfig($(table), 'slice')[1]); | |||
There was a problem hiding this comment.
minor nits:
Maybe move this into a variable?
let tableSlice = readConfig($(table), 'slice');
Maybe rename displayed to displayData. displayed confused me because of the past tense.
@pluehne what do you think?
There was a problem hiding this comment.
I agree with @larsxschneider, displayed could be renamed to displayData for clarity what is actually displayed.
I believe that I have similar code with two readConfig calls as parameters on the same line somewhere else in the code (here and here), so I’ll take the blame for that 🙂. However, this might be the occasion to make the change at the other places as well.
There was a problem hiding this comment.
In this case, we should make the tableSlice constant. Also, I’d rather call it configSlice or something similar to show that it’s part of the configuration and not a slice of the table containing actual data.
There was a problem hiding this comment.
I made the grammar change, but I'll leave the constant tableSlice changes for a separate PR.
docs/repos-feature-usage.html
Outdated
| data-config='{"slice": [0, 50]}' | ||
| ></table> | ||
| <div class="info-box"> | ||
| <p>The top 50 most active repositories, by number of pushes.</p> |
There was a problem hiding this comment.
Is the , here correct?
There was a problem hiding this comment.
I’d say it’s a matter of preferences. Strictly, it isn’t necessary, but a comma can be used to separate additional information (in this case, “by number of pushes”) from the rest of the sentence. In this simple case, I’d leave the comma away.
updater/reports/ReportRepoUsage.py
Outdated
| ON active.repository_id = push_tally.repository_id | ||
| ORDER BY count DESC | ||
| ''' | ||
| return query |
There was a problem hiding this comment.
Nit: In activeReposQuery below you assign query once. Here you append to the query variable. I would only use one stile. I think I prefer "assign once".
There was a problem hiding this comment.
@larsxschneider: I find both styles okay as they are used here. Simple concatenations with + are fine for single variables etc., while entire lines (such as self.activeReposQuery() are broken down with query +=. I think it’s fine as it is 😄.
updater/reports/ReportRepoUsage.py
Outdated
| ( | ||
| SELECT repository_id, COUNT(*) AS count | ||
| FROM pushes | ||
| GROUP BY repository_id |
There was a problem hiding this comment.
Can't you just count the pushes in activeReposQuery? Then we wouldn't need the additional join here. I am not sure if I am missing something here...
|
superseded by #123 |
It is interesting to see what repositories are most active according to Hubble's definition. Moreover, it is very useful to have a complete list of all active repositories. Since this list can be big, display the top 50 on the webpage, and include a link to the complete data set.