-
Notifications
You must be signed in to change notification settings - Fork 19
1335 new bot for monthly statistics #1336
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
base: main
Are you sure you want to change the base?
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.
Very cool addition. I've added a few comments. We should discuss again that we have a workflow here that creates a PR with write permissions and also has access to the secrets.
And it would be great if you could add a few more comments so that the code has a little more structure. Maybe even move some functionalities to seperate files.
| const monthlyData = []; | ||
| const now = new Date(); | ||
|
|
||
| for (let i = 12; i >= 1; i--) { |
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.
rename i to month?
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.
as is used, month would be more confusing i think. i is fine i think
| const [stats, prStats, issueStats, releaseStats] = await Promise.all([ | ||
| getSimpleMonthStats(since, until), | ||
| getPRStats(since, until), | ||
| getIssueStats(since, until), | ||
| getReleaseStats(since, until) | ||
| ]); |
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.
If i understand this correctly, we load the entire history here?
How long does this take?
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.
... for the month. Certainly one of the most intensive tasks, but necessary
| { title: '🐛 Issues Opened', value: latest.issueStats.opened.toString(), short: true }, | ||
| { title: '✅ Issues Closed', value: latest.issueStats.closed.toString(), short: true }, | ||
| ], | ||
| text: `**🏆 Top Contributors:** ${latest.stats.contributorsList.slice(0, 5).join(', ') || 'None'}\n` + |
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 the order here is random. So, this are not the top contributors.
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.
true, also i think this is too competetive, ill just show the contributors
| } | ||
| }); | ||
|
|
||
| for (const commit of commits.slice(0, 30)) { |
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 only the first 30?
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.
Github allows only a certain number of API calls. this is a safe value. i increased it to 50.
| - name: Checkout repository | ||
| uses: actions/checkout@v4 |
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.
is this necessary?
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 is always necessary to get local variables and information of the rep
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write | ||
| issues: write |
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.
where is this used?
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.
It is not anymore, i tried to make this bot do a PR, but that isnt allowed per GH
| const monthLabels = monthlyData.map(d => d.month); | ||
|
|
||
| const charts = [ | ||
| { path: 'docs/stats/total-commits-chart.svg', content: createSVGChart(totalCommits, 'Total Commits', '#3b82f6', monthLabels) }, |
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.
here, we mention 6 svg chart, but only 4 are part of this PR?
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.
true, the new 6 are on the branch which needs to be merged
|
Also, please check the CI runs. |
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.
FOllowing options are available:
- This creates a branch -> manual PR ( no issue, as i think this would be too much overhead)
- This bot pushes directly to the main with permissions, and just updates the pictures (ok i think)
@mknaranja what do you say?
CI now runs fine, don't know why it says failed. Im against separate files as I don't see that we use a different bot with shared functionality and one file is then better. I'll think about comments, but the script structure forces a little bit more of a certain structure.
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write | ||
| issues: write |
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.
It is not anymore, i tried to make this bot do a PR, but that isnt allowed per GH
| - name: Checkout repository | ||
| uses: actions/checkout@v4 |
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 is always necessary to get local variables and information of the rep
| } | ||
| }); | ||
|
|
||
| for (const commit of commits.slice(0, 30)) { |
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.
Github allows only a certain number of API calls. this is a safe value. i increased it to 50.
| const monthLabels = monthlyData.map(d => d.month); | ||
|
|
||
| const charts = [ | ||
| { path: 'docs/stats/total-commits-chart.svg', content: createSVGChart(totalCommits, 'Total Commits', '#3b82f6', monthLabels) }, |
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.
true, the new 6 are on the branch which needs to be merged
| const monthlyData = []; | ||
| const now = new Date(); | ||
|
|
||
| for (let i = 12; i >= 1; i--) { |
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.
as is used, month would be more confusing i think. i is fine i think
| const [stats, prStats, issueStats, releaseStats] = await Promise.all([ | ||
| getSimpleMonthStats(since, until), | ||
| getPRStats(since, until), | ||
| getIssueStats(since, until), | ||
| getReleaseStats(since, until) | ||
| ]); |
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.
... for the month. Certainly one of the most intensive tasks, but necessary
| { title: '🐛 Issues Opened', value: latest.issueStats.opened.toString(), short: true }, | ||
| { title: '✅ Issues Closed', value: latest.issueStats.closed.toString(), short: true }, | ||
| ], | ||
| text: `**🏆 Top Contributors:** ${latest.stats.contributorsList.slice(0, 5).join(', ') || 'None'}\n` + |
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.
true, also i think this is too competetive, ill just show the contributors
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
If need be, add additional information and what the reviewer should look out for in particular:
Merge Request - Guideline Checklist
Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.
Checks by code author
Checks by code reviewer(s)