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

Add a newline normalizing wrapper for io.Readers #340

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

Conversation

carreter
Copy link
Collaborator

@carreter carreter commented Sep 8, 2023

Will be useful for cross-platform support.

Blocked by #339

@Koeng101
Copy link
Contributor

Koeng101 commented Sep 9, 2023

Do you have any specific cases where this is useful? Is it like for, switching \r to \n?

@carreter
Copy link
Collaborator Author

carreter commented Sep 9, 2023

Yes, \r and \r\n to \n.

@Koeng101
Copy link
Contributor

Koeng101 commented Sep 9, 2023

Have you found any bioinformatics files that have \r and are so problematic?

@carreter
Copy link
Collaborator Author

carreter commented Sep 9, 2023

No, but I'm mostly thinking about making sure clients on MacOS, which IIRC we support, can parse files without worrying about normalizing the \r newlines to \n themselves.

@Koeng101
Copy link
Contributor

Koeng101 commented Sep 11, 2023

I see, so it would solve something like #301 . The thing I'm worried about is adding a whole generic intermediate to the parsers - I think it'd reduce readability. The tradeoffs might be worth it if we could support windows though (I use mac pretty often and haven't had this problem)

@carreter
Copy link
Collaborator Author

Hmm. I'm blanking on other ways to structure this that doesn't involve adding an intermediate. IMO we should wire this in in all the NewXXXParserWithMaxLine functions in bio/bio.go, this would minimally impact readability of the format-specific parsers themselves.

@Koeng101
Copy link
Contributor

Koeng101 commented Sep 12, 2023

We can't put it in bio/bio.go though because it would be a circular import, but I'm sure we could put it in some utils location. Once we get ioToBio, we should revisit this for actual integration

@carreter
Copy link
Collaborator Author

I don't think there would be an import cycle. Regardless, I'm going to switch this to a draft PR until #339 is merged. Once it is, this wrapper con go in a new ioutils package:

bio/
├── util.go
└── specificFormat/
    ├── parser.go
    └── types.go
ioutils/
└── util.go

@carreter carreter marked this pull request as draft September 12, 2023 18:48
@carreter carreter added the draft label Sep 16, 2023
@carreter carreter removed the draft label Sep 23, 2023
@github-actions
Copy link

Status: Blocked ❌

Issues blocking this PR:


This comment was automatically written by the Blocking Issues bot, and this PR will be monitored for further progress.

@github-actions github-actions bot added the blocked Waiting for another PR/issue to be merged/closed. label Sep 23, 2023
Copy link

This PR has had no activity in the past 2 months. Marking as stale.

@github-actions github-actions bot added the stale label Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Waiting for another PR/issue to be merged/closed. stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants