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

WIP: Added the option to output contaminant reads to file #44

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SteampunkIslande
Copy link

That's a feature I needed, it would be nice to be able to output contaminant reads in a separate file.

This PR does exactly that, but there are a few points I'd like to discuss with the maintainers:

  • First, I made the tests pass even when mistakingly writing reads with missing line feeds. So I'd like to improve the tests to make sure I didn't break anything.
  • Second, if I understand correctly, the inverse flag of the CLI makes chopper output the 'failing' reads. Maybe we could replace this flag with dedicated file outputs.
  • And last but not least, I am a pretty bad Rust developer, so I didn't find another way of protecting concurrent writes other than that terrible Arc<Mutex<Box<dyn Write + Send>>>. There must be another simpler, more elegant technique ^^

Anyway, thanks for reading this through, have a good day :)

@wdecoster
Copy link
Owner

Hi,

Thanks for contributing! I will look at the code you wrote. Did you perhaps benchmark the time your version takes compared to the latest release?

Best,
Wouter

@SteampunkIslande
Copy link
Author

Did you perhaps benchmark the time your version takes compared to the latest release?

I'm sorry, I didn't. There might indeed be a performance cost, but overall it seems to be working just fine

@SteampunkIslande SteampunkIslande marked this pull request as draft November 21, 2024 09:27
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.

2 participants