tcbrindle / flux

A C++20 library for sequence-orientated programming
https://tristanbrindle.com/flux/
Boost Software License 1.0
476 stars 29 forks source link

Add set sequence adaptors #73

Closed jnytra closed 1 year ago

jnytra commented 1 year ago

Add the following set sequence adaptors that operate on sorted sequences:

Closes #72

jnytra commented 1 year ago

I wrote my first adaptor (set_union). @tcbrindle, if you have time to take a look at my code, I'd be happy for feedback. Then I could continue on the other set adaptors. I apologize for not creating the issue ahead of time. Hopefully you haven't worked on this adaptor already.

tcbrindle commented 1 year ago

Sounds good! Don't worry, I haven't been working on these :)

codecov[bot] commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (1c0dc6e) 97.85% compared to head (7d21175) 97.85%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #73 +/- ## ======================================= Coverage 97.85% 97.85% ======================================= Files 65 65 Lines 2191 2191 ======================================= Hits 2144 2144 Misses 47 47 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

jnytra commented 1 year ago

Thank you very much for your review. I think it has pushed me a lot and I hope I won't make so many mistakes in the future.

tcbrindle commented 1 year ago

Thank you very much for your review. I think it has pushed me a lot and I hope I won't make so many mistakes in the future.

You're not making mistakes :) This stuff is really tricky and most of the information about how it works only exists in my head at the moment, so you've done an amazing job. Also I'm probably being more fussy than usual about style things because Flux is my "baby" and I want it all to be perfect!

tcbrindle commented 1 year ago

I was going through looking at the updated version but I see you're still making changes, so please let me know when it's ready for review again :)

jnytra commented 1 year ago

For now I am done. If you want you can look at it.

jnytra commented 1 year ago

I hope I have corrected all the comments. Thank you for them.

jnytra commented 1 year ago

I added the remaining set adaptors. If you have time, you can take a look at it. @tcbrindle

tcbrindle commented 1 year ago

Looks like my changes in #76 and #77 have broken things, sorry!

jnytra commented 1 year ago

No problem. I will try to fix it.

jnytra commented 1 year ago

Thanks much for review! I hope that I fixed all comments. I am not sure, why Codecov job failed. It seems that I cannot retry it. Also, I do not know how to check if documentation render correctly.

tcbrindle commented 1 year ago

Thanks much for review! I hope that I fixed all comments. I am not sure, why Codecov job failed. It seems that I cannot retry it. Also, I do not know how to check if documentation render correctly.

I've fixed CodeCov in #81 (lcov was updated from 1.16 to 2.0 which broke the our Github Actions script).

Please could you merge the latest upstream changes and try again? Github will allow me to do it, but updating someone else's repository feels a bit rude!

jnytra commented 1 year ago

The latest upstream changes merged. Now all jobs passed.

tcbrindle commented 1 year ago

Sorry it's taken me a few days to get to this, it's a lot of code to look through! But it all looks really great. Not just one but four tricky adaptors, you've done a great job!

jnytra commented 1 year ago

Nothing happens, I was on vacation anyway. Thank you very much for the review.