Skip to content

Return group errors #110

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

Merged
merged 6 commits into from
Apr 17, 2023
Merged

Conversation

armanio123
Copy link

Returns #105

Fixes the issue caused by this same PR as well. The bug was caused by tmpfs being unmounted before using the files for reporting. Now, we read all of the necessary files before unmounting.

I appreciate if I can get a review on the latest commit on this PR. (The one that introduces the fix).

src/main.ts Outdated
Comment on lines 463 to 464
console.log("Extracting commit SHA for repro steps");
const commit = (await execAsync(summary.repoDir, `git rev-parse @`)).trim();
text += `<li>In dir <code>${summary.repo.name}</code>, run <code>git reset --hard ${commit}</code></li>\n`;
text += `<li>In dir <code>${summary.repo.name}</code>, run <code>git reset --hard ${summary.commit}</code></li>\n`;
Copy link
Member

Choose a reason for hiding this comment

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

This will end up being a behavior change; this try/catch was for the exec but now it doesn't do anything here, but can do something in the place it was moved to. I would try catch over in the new place to preserve this behavior.

Copy link
Author

Choose a reason for hiding this comment

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

You're correct. Fixed. Instead, I now have the try-catch where the commit is extracted and also assigned a nullable value in case we don't get it.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

I think this is good for a re-land? Not sure we can test it before running it on another PR.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Apr 17, 2023

We can only test it if it's on a branch on the repo itself.

https://github.com/microsoft/typescript-error-deltas/tree/eae0447d395ee6d3330f5df751984044070dca3f#running

Armando Aguirre and others added 2 commits April 17, 2023 15:43
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
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.

3 participants