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

Status Chart: Add GitHub PR/issue/bug data #1038

Merged
merged 6 commits into from
Jul 13, 2020

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Jul 12, 2020

  • Switch from Moment.js to date-fns.
    • date-fns loads faster and is more modern. This will make it easier to upgrade to Chart.js v3, which won't bundle Moment.js.
    • We need to adjust our format strings to use Unicode tokens, required by date-fns.
  • Make the Toggle Timeframe button larger.
  • Mention that the legend is clickable.
  • Add 2020-07-10 row.
  • Add historical GitHub PR/issue/bug data.
    • To avoid confusion, rename the key for old 'bugs' to 'vso'.
    • Add trailing commas to the table's key/value pairs.
    • Add/recolor datasets, and rename 'Bugs' to 'Old Bugs'.
    • Update axis labels to mention Issues and Pull Requests.
    • Add/update explanations. Reorder them to match the legend.

Live preview: stephantlavavej.github.io/STL/

I am aware of the potential colorblind accessibility issue of having both red and green lines. I'm unsure whether this is a major problem, or how best to mitigate it. The chart is interactive (unlike the old static screenshots of an Excel chart), so toggling line visibility will reveal which is which. Additionally, Chart.js has dashed line styles that also appear in the legend. I am considering making the Old Bugs and GitHub Bugs dashed in a future update.

Finally, I obtained this historical data by becoming even more of a JavaScript programmer! 😸

// Copyright (c) Microsoft Corporation.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

const { DateTime } = require('luxon');
const { Octokit } = require('@octokit/rest');

function has_label(elem, label_name) {
    for (const label of elem['labels']) {
        if (label['name'] === label_name) {
            return true;
        }
    }
    return false;
}

const octokit = new Octokit({
    auth: 'FIXME_PERSONAL_ACCESS_TOKEN',
});

octokit.paginate(octokit.issues.listForRepo, {
    owner: 'microsoft',
    repo: 'STL',
    state: 'all',
}).then(api_output => {
    console.log(`Received ${api_output.length} PRs and issues.`);

    const begin = DateTime.fromISO('2019-08-30' + 'T23:00:00-07');
    const now = DateTime.local();

    for (let when = begin; when < now; when = when.plus({ weeks: 1 })) {
        let num_pr = 0;
        let num_issue = 0;
        let num_bug = 0;

        for (const element of api_output) {
            const opened = DateTime.fromISO(element['created_at']);
            const closed = DateTime.fromISO(element['closed_at'] ?? '2100-01-01');

            if (when < opened || closed < when) {
                // was not active; do nothing
            } else if (element['pull_request'] !== undefined) {
                ++num_pr;
            } else if (has_label(element, 'cxx20')) {
                // avoid double-counting C++20 Features and GitHub Issues
            } else {
                ++num_issue;

                if (has_label(element, 'bug')) {
                    ++num_bug;
                }
            }
        }

        console.log(`date: '${when.toISODate()}', pr: ${num_pr}, issue: ${num_issue}, bug: ${num_bug}`);
    }
});

date-fns loads faster and is more modern. This will make it
easier to upgrade to Chart.js v3, which won't bundle Moment.js.

We need to adjust our format strings to
use Unicode tokens, required by date-fns.
To avoid confusion, rename the key for old 'bugs' to 'vso'.

Add trailing commas to the table's key/value pairs.

Add/recolor datasets, and rename 'Bugs' to 'Old Bugs'.

Update axis labels to mention Issues and Pull Requests.

Add/update explanations. Reorder them to match the legend.
@StephanTLavavej StephanTLavavej added the documentation Related to documentation or comments label Jul 12, 2020
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner July 12, 2020 10:06
@AlexGuteniev
Copy link
Contributor

The chart is interactive (unlike the old static screenshots of an Excel chart), so toggling line visibility will reveal which is which.

This part is not very discoverable (I see colors, but still find it useful to hide axes). Explicit checkboxes could have been better.

Also maybe split legend by left axis / right axis part and a put small gap. So that it would look less like "only seven GihHub bugs"

index.html Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member Author

Thanks for reviewing, @AlexGuteniev - it really helps to have another perspective on how comprehensible this thing is.

This part is not very discoverable

Agreed. I'll mention that the legend is clickable.

Explicit checkboxes could have been better.

Yeah, the lack of affordance is an issue. That might be possible with Chart.js's legend configuration but that's beyond my current abilities. (I don't know if legend labels can be controlled separately from dataset labels.)

Also maybe split legend by left axis / right axis part and a put small gap.
So that it would look less like "only seven GitHub bugs"

Adding a gap is also beyond my abilities. (I don't see how to control horizontal padding, or any mechanism for creating two groups.)

I thought about the axis placement a lot - I want the features to occur first in the legend, both because they're "important" and because this gives them z-axis priority. However, I also prefer the feature/small axis to appear on the right, since that's where we naturally look for the current status. So there's left/right reversal that I'm not really fond of.

One response would be to admit defeat and simply generate two charts, instead of having two y-axes. (The same effect can be manually obtained by hiding all of the left-axis or right-axis lines, since display: 'auto' hides unused axes.) I like having the unified view, though. Another response would be to move the feature/small axis to the left (which wouldn't be too bad; they are intentionally scaled by exactly 10x).

@StephanTLavavej StephanTLavavej merged commit 070833b into microsoft:gh-pages Jul 13, 2020
StephanTLavavej added a commit to StephanTLavavej/STL that referenced this pull request Jul 19, 2020
Fixes microsoft#1043.

* `gather_stats.js`
  + This uses the GitHub API to gather PR/issue statistics. It runs on the console (not in the browser), using the latest version of Node.js (currently 14.5.0). I've significantly improved it since my initial version in microsoft#1038 that wasn't checked into the repo:
    - This loads a personal access token from a `.env` file, avoiding the danger of hardcoding it in the script.
    - Because it currently takes about 20 seconds to list all PRs/issues through the GitHub API, the script displays a command-line progress bar.
    - For efficiency, the script transforms PR/issue data as it's received (instead of repeatedly during analysis).
    - The script now analyzes the data with daily resolution. Accordingly, I've adjusted the beginning date.
    - The script now writes a file, `daily_table.js`, complete with a "generated file" warning comment.
    - The script now calculates combined PR age.
* `daily_table.js`
  + The generation process is stable, so regenerating this file will typically add rows to the bottom with no other changes. However, it's possible for previous rows to change (e.g. due to relabeling or reopening).
* `weekly_table.js`
  + I extracted the classic table, still updated manually, for symmetry. This also makes the source and history of `index.html` easier to read. Eventually, I plan to generate the `cxx20` and `lwg` stats, and we'll stop tracking `vso` after porting all old bugs, leaving only `libcxx` that can't be easily generated yet.
* `.gitignore`
  + We need to ignore the `node_modules` directory that `npm` creates. We also need to ignore the `.env` file, but I can't tell you why - it's a secret.
* `package.json`
  + This is generated by `npm init` and is updated when dependencies are installed. I filled out some reasonable information in response to its questions, but I don't intend to publish this in the registry (as it is hardcoded for our repo and not intended for general use, although it could serve as the basis for such work).
* `package-lock.json`
  + This is also a generated file, and I don't really understand its purpose, except that it's supposed to be checked in (something about pinning the exact versions of all transitive dependencies).
* `index.html`
  + Add `hr` styling, to separate the two charts.
  + Include `daily_table.js` and `weekly_table.js`.
  + Move the main `<script>` into the `<head>`, making it easier to read the logic versus the content. (This required only one change, moving the "Toggle Timeframe" button's `addEventListener` to `window.onload`, which is invoked after the page has finished loading.)
  + Generalize `get_values` because there are two tables now.
  + Rename `chart_data` to `status_data` (and similarly for `chart_options`) because there are two charts now.
  + Add `age_data`.
  + Give `timeframe_all` and `timeframe_github` names, making it easier to refer to them later (`timeframes[0]` was unnecessarily mysterious).
  + Extract `common_options` used by both charts.
  + Extract a `make_xAxes` function, as both charts use the same x-axis, but with different timeframes.
  + `...common_options` is "spread syntax".
  + Add `age_options` for the new chart. Note that attempting to centralize their `title` configuration (where `display` and `fontSize` are the same, but `text` is different) seems like more trouble than it's worth.
  + Construct the `age_chart`, and move the button logic as previously mentioned.
  + Add the `<canvas id="ageChart">` followed by an explanation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation or comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants