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

Experiment: Add total duration to the agenda #1706

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Oct 2, 2024

image

Lets see if this works -- I'm not sure about how GitHub caches images.

Server code (using temporal and iterator helpers 😄): https://dash.deno.com/playground/tc39-agenda-time

const pattern = new URLPattern({ pathname: "/:year/:month{/}?" });

Deno.serve(async (req: Request) => {
    try {
        const { year, month } = pattern.exec(req.url).pathname.groups;

        const res = await fetch(`https://raw.githubusercontent.com/tc39/agendas/refs/heads/main/${year}/${month}.md`);
        const body = await res.text();

        let outputText;

        if (res.status === 404) {
            outputText = "Err 404"
        } else if (!res.ok) {
            return new Response("Error while fetching the agenda:\n\n" + body, {
                status: res.status,
                headers: { "content-type": "text/plain" }
            });
        } else {
            const minutes = body.matchAll(/\b(\d+)m(?=\s*[,|)])(?!\) Timeboxed Discussions)/g)
                .map(m => parseInt(m[1], 10))
                .reduce((sum, m) => sum + m, 0);

            const duration = Temporal.Duration.from({ minutes }).round({ largestUnit: "hours" });

            outputText = `${duration.hours}h ${duration.minutes}m`;
        }

        return new Response(`
            <svg version="1.1" width="80" height="20" xmlns="http://www.w3.org/2000/svg">
                <style>
                    @media (prefers-color-scheme: dark) {
                        text {
                            fill: white;
                        }
                    }
                </style>
                <text x="0" y="18" font-family="sans-serif">${outputText}</text>
            </svg>
        `, {
            headers: { "content-type": "image/svg+xml", "cache-control": "no-cache, max-age=0" }
        })
    } catch {
        return new Response("Error", { status: 500 });
    }
});

If this works, then I can add it to the template.

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review October 2, 2024 11:52
@nicolo-ribaudo
Copy link
Member Author

Should we extract only the durations in the tables, or (which is what this PR is doing) also those in the bullet points?

@ljharb
Copy link
Member

ljharb commented Oct 2, 2024

Github caches images very aggressively, and I'd think we want to avoid adding more external dependencies.

@ljharb ljharb marked this pull request as draft October 2, 2024 14:32
@nicolo-ribaudo
Copy link
Member Author

Wdyt about an action that updates the time whenever there is a new commit on main?

@ljharb
Copy link
Member

ljharb commented Oct 2, 2024

That seems like it'd create a ton of noise commits :-/

@bakkot
Copy link
Contributor

bakkot commented Oct 2, 2024

Are we worried about the number of commits in this repo? I certainly would prioritize having the total time readily available over minimizing the number commits, personally.

@ljharb
Copy link
Member

ljharb commented Oct 2, 2024

In general yes, git commit history is worth keeping clean on every repo.

I'm sure we can come up with a workaround - a github pages page that calculates it, for example, or a CI check that blocks additions that exceed the total time - that doesn't require spamming commits to the default branch.

@nicolo-ribaudo
Copy link
Member Author

It seems like the cache-control header is working properly to prevent GitHub from caching the image — the preview of this PR already updated the total time due to #1707

@nicolo-ribaudo
Copy link
Member Author

I just updated the Deno script because the regexp was accidentally including 30m from "Short (≤30m) Timeboxed Discussions", and the PR preview was updated immediately. Caching is definitely not a problem :)

@ryzokuken
Copy link
Member

It doesn't work as well for dark mode
image

@nicolo-ribaudo
Copy link
Member Author

@ryzokuken Fixed

Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great now

@bakkot
Copy link
Contributor

bakkot commented Oct 4, 2024

For alt text, something like "dynamically computed total of timeboxes on the agenda"?

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Oct 8, 2024

Alt text updated. Also I figured out how to change the URL to something nicer.

@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review October 8, 2024 01:45
@michaelficarra
Copy link
Member

Note that this will possibly show the wrong duration when looking at older versions of the agenda.

@ljharb
Copy link
Member

ljharb commented Oct 8, 2024

how so? the year and month are part of the URL

@michaelficarra
Copy link
Member

@ljharb Not other agenda documents. An older commit.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Oct 8, 2024

Yeah this will always fetch the duration from main. That part of the agenda is mostly meant for us so it's probably fine, but I can add an explicit mention of this.

Maybe one day with Shadow realm we'll be able to directly run JS in markdown :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants