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.

Sign up for GitHub

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

Jump to bottom

Allow configuring the number of threads #173

Merged
merged 2 commits into from
Feb 9, 2022
Merged

Allow configuring the number of threads #173

merged 2 commits into from
Feb 9, 2022

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Oct 8, 2021 •
edited
Loading

  • Tests...

@XhmikosR XhmikosR force-pushed the configure-threads branch 18 times, most recently from e964c7d to 3ae7a86 Compare October 14, 2021 07:41
@XhmikosR XhmikosR requested a review from nschonni October 24, 2021 13:43
@XhmikosR XhmikosR marked this pull request as ready for review October 24, 2021 13:46
XhmikosR
XhmikosR commented Oct 24, 2021
View reviewed changes
lib/getThreads.js Outdated
return threads;
}

throw new Error(`Invalid threads option! (${threads})`);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if throwing here is a good idea since it seems to break --force, but I think it's better than swallowing the error. Unsure how to do this the other way.

Copy link
Member

Choose a reason for hiding this comment

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

What about a plain console.warn about the bad config, then just use the default of 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

@nschonni I'm not sure I follow the latter part of your comment. Could you please commit your suggestion?

Copy link
Member Author

Choose a reason for hiding this comment

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

NVM, I got what you mean. If you could push your suggestion, that would help , otherwise I'll see when I find some time to finish this.

@XhmikosR XhmikosR force-pushed the configure-threads branch 3 times, most recently from bd66b54 to 2117982 Compare February 9, 2022 07:20
@XhmikosR XhmikosR merged commit e6993a4 into main Feb 9, 2022
@XhmikosR XhmikosR deleted the configure-threads branch February 9, 2022 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nschonni nschonni Awaiting requested review from nschonni

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@XhmikosR @nschonni