tiagobento commented on code in PR #2099:
URL:
https://github.com/apache/incubator-kie-tools/pull/2099#discussion_r1434401311
##########
packages/chrome-extension/src/app/github/api.ts:
##########
@@ -39,7 +45,7 @@ export function fetchFile(
.then((res) => (contentType === ContentType.BINARY ? (res.data as
any).content : atob((res.data as any).content)))
.catch((e) => {
console.debug(`Error fetching ${path} with Octokit. Fallback is
'raw.githubusercontent.com'.`);
- return
fetch(`https://raw.githubusercontent.com/${org}/${repo}/${commitSHA}/${path}`).then((res)
=>
+ return fetch(modifiedRawGithubUserContentUrl.toString()).then((res) =>
res.ok ? res.text() : Promise.resolve(undefined)
);
});
Review Comment:
I don't see why we would need `commitSHA` in this case.... I mean, there's
only one place that looks for the `commitSHA` and it is here:
> @ prEditors.tsx#parsePrInfo
` const commitSHA =
dependencies.all.getViewFileButton()?.href.split("/")[6];`
Now, let's analyze everything...
1. Looking at the GitHub PR page, it looks like
`dependencies.all.getViewFilebutton()` is working as expected.
<img width="632" alt="image"
src="https://github.com/apache/incubator-kie-tools/assets/1584568/201c0d8c-9860-40f2-af4e-a368baf72800";>
2. Since there can be multiple "View file button"s in the PR page, I already
think we should change this name to `getFirstViewFileButton`. Getting the
`commitSHA` out of it is ok too, as it doesn't matter how many buttons will
there be, since they'll always have the same `commitSHA` to be extracted.
3. Taking a step further, and seeing if the `commitSHA` extraction works as
expected... it does:
<img width="586" alt="image"
src="https://github.com/apache/incubator-kie-tools/assets/1584568/ca842e10-1fba-466f-b16e-d77db7d8240f";>
4. Ok, so why would `commitSHA` not exist? Maybe because we're looking at an
Editor that's not inside a PR? :D From your video it looks like we're dealing
with the <SingleEditorViewApp>, which uses `fetchFile` without `commitSHA`.
---
Anyway...... Since it does work with `ref` and `commitSHA` is only used in
that specific place.. I think we should be removing this whole `commitSHA`
thing. WDYT? :)
##########
packages/chrome-extension/src/app/github/api.ts:
##########
@@ -29,6 +29,12 @@ export function fetchFile(
contentType?: ContentType,
commitSHA?: string
) {
+ const rawGithubUserContentUrl = "https://raw.githubusercontent.com";;
+ const modifiedRawGithubUserContentUrl = new URL(rawGithubUserContentUrl);
+ modifiedRawGithubUserContentUrl.pathname = `${org}/${repo}/${ref}/${path}`;
+ if (commitSHA) {
+ modifiedRawGithubUserContentUrl.pathname =
`${modifiedRawGithubUserContentUrl.pathname}/${commitSHA}`;
+ }
Review Comment:
If you agree with removing the `commitSHA` altogether, this code will change
a lot, but as I'm here, here's some suggestions on how to make this better.
1. If `commitSHA` exists, in this case your final URL would be
``${org}/${repo}/${ref}/${path}/${commitSHA}`. Which is NOT preserving the old
behavior.
2. The `rawGithubUserContentUrl` variable is totally unnecessary. You could
inline it to avoid having an extra line and having to name one extra thing as
well (although this is perfectly named, IMHO).
3.
```typescript
modifiedRawGithubUserContentUrl.pathname = `${org}/${repo}/${commitSHA ??
ref}/${path}`
```
4. All this logic is being executed before you're even sure you'll need it.
Always try to put stuff inside the smallest scope possible (unless you have
bigger plans). In this case, this entire logic only applies to the block inside
the `catch` there.
would be much better, IMHO.
Anyway, if we agree to remove the commitSHA stuff, this can be reduced a lot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
