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

added suspending for rendering #23

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

Conversation

bunysae
Copy link

@bunysae bunysae commented Sep 3, 2019

In order to fix issue np issue 79 these changes suspend the update-process, if the task set a suspend flag.

@sindresorhus
Copy link

You need to fix the lint errors. Run $ npx xo --fix locally.

index.js Outdated
return task.shouldSuspendUpdateRenderer();
}
}
}

Choose a reason for hiding this comment

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

I feel like this logic belongs in listr and not the renderer.

Copy link
Author

Choose a reason for hiding this comment

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

After writing the tests, i remove it to listr.

Copy link
Author

Choose a reason for hiding this comment

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

This logic has been removed to listr with the commit 4685274.

@sindresorhus
Copy link

This PR needs to add a test to prove it works correctly.

index.js Outdated
@@ -44,8 +44,19 @@ const renderHelper = (tasks, options, level) => {
return output.join('\n');
};

const shouldSuspendUpdateRenderer = tasks => {
for (const task of tasks) {
if (task.shouldSuspendUpdateRenderer()) {

Choose a reason for hiding this comment

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

Maybe use tasks.some() here to simplify the code?

Copy link
Author

Choose a reason for hiding this comment

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

Done

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