Closed piotrpdev closed 3 weeks ago
I found that in the current changes, we only need the number of the PR, not the whole URL of the PR. Moreover, the code has a lot of handling of URLs (fetching the PR number through URLs), so can we consider changing
pr-url
topr-id
?
@BlackHole1 Sorry, can you be a bit more specific on where you want to change pr-url
to pr-id
? Are you talking about making that change in the schema (https://github.com/electron/rfcs/pull/6), or just in some of the code in this PR?
Personally, I like pr-url
and dislike changing it to pr-id
. I like the history data being as close to the original API History block as possible. Using URLs also doesn't restrict us to just electron/electron
which could be useful in the future.
It's also useful in scenarios like these:
const formattedVersions = allVersions.map((version) => {
return (
<a
key={version}
href={change['pr-url']}
target="_blank"
rel="noopener noreferrer"
>
{/* Semver shenanigans, feature backported to both ^7.1.0 and ^6.3.0 would not be present in 7.0.0 */}
<pre>
{release === version ? '>=' : '^'}
{version}
</pre>
</a>
);
});
Before answering this question, I need to apologize first. Due to work and time zone differences, I wasn't able to participate in the RFC discussions and Slack discussions, which caused me to miss a lot of context.
@BlackHole1 Sorry, can you be a bit more specific on where you want to change pr-url to pr-id? Are you talking about making that change in the schema (https://github.com/electron/rfcs/pull/6), or just in some of the code in this PR?
I recently discovered during the code review of this PR that the current changes almost do not use the pr-url, but only use the pr-id within it. To get the pr id, there are many lines of code like Number(added['pr-url'].split('/').at(-1))
. I believe this is redundant code.
I looked through https://github.com/electron/rfcs/pull/6, but did not see any relevant discussion, so I am raising this question here.
Using URLs also doesn't restrict us to just electron/electron which could be useful in the future.
Although I don't see when support will be extended to repositories outside of electron/electron in the short term, I can reluctantly accept this reason.
It's also useful in scenarios like these: https://github.com/electron/website/pull/594/files#diff-1dd7a747a324f6f8916a65d87d4c435b9041ed1b0a0d9fa48518423b4d3012edR42-R57
Yes, pr-url is only used here. My question is, why not use pr-id directly and then use something like: https://github.com/electron/electron/pulls/${prID}
here? Wouldn't this reduce the redundancy in the code overall?
Overall, this PR, CODE LGTM. Since other members think pr-url is acceptable, I won't strongly oppose it, just raising a question :)
I think this is ready. 🎉 @piotrpdev, can you go ahead and remove the docs/
changes? Then I'll go ahead and merge.
@dsanders11 I think this is ready. 🎉 @piotrpdev, can you go ahead and remove the
docs/
changes? Then I'll go ahead and merge.
Done, hopefully nothing breaks on merge 🤞
Use
export GH_TOKEN=<token>
beforeyarn start
if you want to test the releases fetching and unzipping.You might have to manually modify
sidebars.js
to makeyarn start
work.TODO
docs/api/*.md
before merge, they're just for previewing/testing the PR.src/transformers/api-history.ts
https://github.com/electron/website/pull/594/files#diff-1150b9be07a8f0d779823f74f0403681feb5bc13bfda0a500edeadd81a8ce61cR94-R113Replace fetching the PR release versions artifact with fetching a JSON file once @dsanders11 implements that.Won't happen for a while, will merge withGH_TOKEN
.