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

feat: add callback to be able store compare result information #242

Conversation

wokayme
Copy link
Contributor

@wokayme wokayme commented Sep 14, 2022

Example of implementation for following idea:
#241


const secondCompareResult = await makeCompareCall(false);
if(afterComparison && typeof afterComparison === "function") {
afterComparison(secondCompareResult);
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to add the callback on line 52 as well. That's the more common case, right here is where there is a compareThreshold used. Which is rare.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added also condition for isAsync

@@ -108,7 +108,13 @@ export default async function compareReports(
// Make second compare call to finalize the deep compare. The second call will
// cause a status to be posted to the PR (if applicable). Any ignored diffs
// from the first call will be excluded from the result.
const secondCompareResult = await makeCompareCall(false);
log(secondCompareResult.summary);
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this line down as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As now?

@wokayme wokayme force-pushed the feat(comparison)--add-callback-to-be-able-store-compare-result-information branch from d2a94a9 to cfd4a5c Compare September 14, 2022 08:17
Copy link
Contributor

@trotzig trotzig left a comment

Choose a reason for hiding this comment

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

Looks good! I don't have time to merge and release this today but I'll make sure it gets through tomorrow. Thanks for taking the time to contribute!

@@ -48,6 +48,10 @@ export default async function compareReports(
typeof compareThreshold === 'number' || dryRun,
);
if (typeof compareThreshold !== 'number' || isAsync) {

if(!isAsync && afterSyncComparison && typeof afterSyncComparison === "function") {
afterSyncComparison(firstCompareResult);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we await the results so that you can run an asynchronous function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ;)

@wokayme
Copy link
Contributor Author

wokayme commented Sep 14, 2022

I am happy that I could help. And I am waiting for tomorrow release ;)

@trotzig trotzig merged commit c6ea2b4 into happo:master Sep 14, 2022
@trotzig
Copy link
Contributor

trotzig commented Sep 14, 2022

This was released in version 8.1.0. 🎉

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

2 participants