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

Shrink RELEASES.md #112257

Closed

Conversation

Mark-Simulacrum
Copy link
Member

This is a mostly1 identically rendered result, while cutting 9.4% (59.5 kilobytes, roughly 6 releases worth) from the file. My hope is that this puts us back into territory where GitHub is rendering the file without issues, giving us some more time to figure out next steps for shrinking/adjusting future releases.

Tested with manual inspection of HTML diffs generated by pulldown-cmark. See the inlining script embedded into the "Inline [...]" commit for how I did the replacement.

cc @ehuss

r? @cuviper

Footnotes

  1. In practice, we had a bunch of references that collided previously or where duplicated with different links (e.g., one to nightly and one to stable). The commits here clean those cases up while we're touching this so that there's less manual inspection needed.

These links are either dead anyway (e.g., if they pointed at now-deleted functions or similar in
/nightly/) or should still work with latest stable. This cuts ~6.5 kilobytes from the size of the
file.
This prepares us for replacing the out-of-band references with inline URLs, which saves a bunch of
space in the markdown file.
This cuts 8.3% (51KB) from the file, in the hopes of keeping the current version renderable by
GitHub without breaking links. This is not a sustainable solution into the future, but it seems like
we need something to this effect to at least keep past links going to the same content.

Changes manually verified by converting to HTML before/after with pulldown-cmark. Some of the
previous commits help make this a near-noop to make this verification much easier.

Small hand-written inliner used for the replacement:

```
fn main() {
    let mut defs = std::collections::HashMap::new();
    let file = std::fs::read_to_string("RELEASES.md").unwrap();
    let mut new = String::new();
    for line in file.lines() {
        if line.starts_with('[')
            && line.contains("]: ")
            // Very common.
            && !line.starts_with("[platform-support-doc]")
        {
            let key = &line[..line.find(']').unwrap() + 1];
            let value = line.strip_prefix(key).unwrap()[":".len()..].trim_start();
            if let Some(old_value) = defs.insert(key, value) {
                if old_value != value {
                    eprintln!(
                        "Warning: {:?} defined multiple times ({:?} != {:?})",
                        key, old_value, value
                    );
                }
            }
            // Definitions aren't pushed, we're substituting all of them.
        } else {
            new.push_str(line);
            new.push('\n');
        }
    }

    for (key, value) in defs.iter() {
        new = new.replace(&format!("]{}", key), &format!("]({})", value));

        // Replace cases where the key is directly in-text rather than a [text][key] reference.
        for ch in ['\n', '.', ',', '\'', ' ', ')'] {
            new = new.replace(&format!("{}{}", key, ch), &format!("{}({}){}", key, value, ch));
        }
    }

    std::fs::write("RELEASES.md.new", &new).unwrap();
}
```
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-release Relevant to the release subteam, which will review and decide on the PR/issue. labels Jun 3, 2023
@jeffparsons
Copy link
Contributor

Surely someone in the Rust org knows someone on the GitHub team. I'm not suggesting that they would/should do all the work to migrate to a better renderer just because it would help Rust... but maybe they could at least offer a more generous timeout to a reputable org like rust-lang...

I've had multiple SaaS providers add these kinds of exceptions for teams I'm on in the past, so I wouldn't assume they'll say no until they've actually said no.

@est31
Copy link
Member

est31 commented Jun 4, 2023

Not sure if still done but there is evidence on zulip that there are regular calls with github staff happening by Rust representatives.

@cuviper
Copy link
Member

cuviper commented Jun 5, 2023

Large diffs are not rendered by default.

/me hits "Load diff"

Oops, something went wrong.

Oh, the irony...

@cuviper
Copy link
Member

cuviper commented Jun 5, 2023

More seriously, I don't really see a good way to review this. I can fetch the patch locally, but it's a lot of rote replacement to be reviewing by hand. I trust your own verification via pulldown-cmark, but since the new file is still failing to render, we can't really see it "live". And if we haven't actually bought any time there, I'm not sure there's any point in the churn.

(Some of your changes are probably good fixes on their own merit though.)

@Mark-Simulacrum
Copy link
Member Author

Yeah. I think you can best review commit by commit and checking with pulldown-cmark yourself (similar to what I did myself), but I agree it may not be worth it since this doesn't seem to fix the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants