Skip to content

Conversation

salbert83
Copy link
Contributor

Perhaps it is better to let the client decide if they wish to use multithreading?

Perhaps it is best to let the client decide whether they wish to use multithreading?
@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2022

Codecov Report

❗ No coverage uploaded for pull request base (dev@72690a0). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             dev     #176   +/-   ##
======================================
  Coverage       ?   89.67%           
======================================
  Files          ?       10           
  Lines          ?     1182           
  Branches       ?        0           
======================================
  Hits           ?     1060           
  Misses         ?      122           
  Partials       ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72690a0...f16efd7. Read the comment docs.

@ablaom ablaom requested a review from OkonSamuel June 26, 2022 20:42
@ablaom
Copy link
Member

ablaom commented Jun 26, 2022

@OkonSamuel This replaces #175. Can you please review?

@OkonSamuel
Copy link
Member

Perhaps it is better to let the client decide if they wish to use multithreading?

Yes. But we could always include support for that later.

Copy link
Member

@OkonSamuel OkonSamuel left a comment

Choose a reason for hiding this comment

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

LGTM except for some little changes

salbert83 and others added 2 commits June 27, 2022 22:53
Good idea!

Co-authored-by: Okon Samuel <[email protected]>
@OkonSamuel
Copy link
Member

@salbert83 everthing is good to go. Could you just add a test for use_multithreading

@ablaom
Copy link
Member

ablaom commented Jul 8, 2022

@salbert83 Are you willing to add a test (see comment above)?

@salbert83
Copy link
Contributor Author

salbert83 commented Jul 8, 2022 via email

@salbert83 salbert83 requested a review from OkonSamuel July 9, 2022 00:51
@salbert83
Copy link
Contributor Author

Any suggestion on resolving the conflict?

@ablaom ablaom changed the base branch from master to dev July 14, 2022 08:26
@ablaom
Copy link
Member

ablaom commented Jul 14, 2022

Probably the safest suggestion is for you to try merging latest origin/dev into your local branch and then pushing the updated branch to your fork. There will be conflicts to resolve locally in the process. If you get stuck with that, or am not confident trying this, let me know and I will have a look at resolving the conflicts for you.

@ablaom
Copy link
Member

ablaom commented Jul 18, 2022

The commit history still looking suspicious. Closing in favor of #188

@ablaom ablaom closed this Jul 18, 2022
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.

5 participants