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

Fix: Call formats asynchronously #880

Closed
wants to merge 1 commit into from
Closed

Fix: Call formats asynchronously #880

wants to merge 1 commit into from

Conversation

molant
Copy link
Member

@molant molant commented Mar 13, 2018

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

There were some reports that Excel formatter was generating just an empty file. It turns out it's the first formatter that actually requires to be called asynchronously.

This PR makes the call to formatter.format using await.

if (hasError(reports)) {
endSpinner('fail');
} else {
endSpinner('succeed');
}

sonarwhal.formatters.forEach((formatter) => {
formatter.format(reports, target);
await each(sonarwhal.formatters, async (formatter) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you use the package async instead of use a for … of..?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should start using async in all places instead of for … of for legibility purposes and this was a good moment to start.

Copy link
Contributor

Choose a reason for hiding this comment

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

for … of is legible :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's replace all the map, reduce, forEach, etc. for regular for loops then 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not the same example, you don't need to add any extra package to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are using lodash in parts of the code 🙄

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