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

feature: Add descriptive check title & pull request comment summary #65

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jarojasm95
Copy link

@jarojasm95 jarojasm95 commented Dec 22, 2021

This PR adds a descriptive check title when there is only a single report being analyzed. Additionally it changes the pull request comment body to use a <summary/> block

@jarojasm95 jarojasm95 changed the title feature: Add descriptive check title feature: Add descriptive check title & pull request comment summary Dec 23, 2021
@@ -72,13 +75,20 @@ async function action(payload) {
const belowThreshold = reports.some(
(report) => Math.floor(report.total) < minimumCoverage
);

let reportTitle = reportName;
// TODO: Figure out a good title for more than one report
Copy link
Author

Choose a reason for hiding this comment

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

ideas/suggestions welcome 🙂

await client.rest.checks.create({
name: checkName,
head_sha: sha,
status: "completed",
conclusion: conclusion,
output: {
title: checkName,
title: checkTitle,
Copy link
Author

@jarojasm95 jarojasm95 Dec 23, 2021

Choose a reason for hiding this comment

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

this distinction between check name and title is so that the title can be more descriptive and look like this:
Screen Shot 2021-12-22 at 6 23 29 PM

}
const minimumCoverageText = `_Minimum allowed coverage is \`${minimumCoverage}%\`_`;
const footerText = `<p align="right">${credits} against ${commit} </p>`;
output += `${minimumCoverageText}\n\n${footerText}`;
return output;
return [output, structuredOutput];
Copy link
Author

Choose a reason for hiding this comment

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

the second output value is the one that contains the <summary/> block best suited for a PR comment:
Screen Shot 2021-12-22 at 6 25 36 PM
Screen Shot 2021-12-22 at 6 25 45 PM

Copy link
Author

Choose a reason for hiding this comment

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

I could see this being an additional input parameter for the action, possibly called collapse_pr_comment: bool (or something like that)

@scottrice10
Copy link

scottrice10 commented Jul 18, 2022

Any update here? Changing to make the file coverage list would be great, as for large projects, the number of files shown is too long and unwieldly.

@Swamii
Copy link

Swamii commented Sep 29, 2022

Any update here? Changing to make the file coverage list would be great, as for large projects, the number of files shown is too long and unwieldly.

Side note: I use only_changed_files for older projects to avoid the wall of text.

@Swamii
Copy link

Swamii commented Sep 29, 2022

@jarojasm95 Thanks for the PR 🙏 sorry for the late reply.

I could see us merging this if the PR becomes a bit more focused. That means only focusing on the check titles. So no extraneous settings added (pull_request_comment and check_name). I would at least like to see an issue with a discussion about that, to see that there is a need before we add it.

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.

None yet

3 participants