Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add a newline normalizing wrapper for io.Readers#340

Draft
carreter wants to merge 1 commit intobebop:mainfrom
carreter:normalize_newlines
Draft

Add a newline normalizing wrapper for io.Readers#340
carreter wants to merge 1 commit intobebop:mainfrom
carreter:normalize_newlines

Conversation

Copy link
Collaborator

carreter commented Sep 8, 2023 *
edited
Loading

Will be useful for cross-platform support.

Blocked by #339

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?

Copy link
Collaborator Author

carreter commented Sep 9, 2023

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

Copy link
Contributor

Koeng101 commented Sep 9, 2023

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

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.

Copy link
Contributor

Koeng101 commented Sep 11, 2023 *
edited
Loading

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 mentioned this pull request Sep 12, 2023
Copy link
Collaborator Author

carreter commented Sep 12, 2023

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.

Copy link
Contributor

Koeng101 commented Sep 12, 2023 *
edited
Loading

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

Copy link
Collaborator Author

carreter commented Sep 12, 2023

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
Koeng101 reacted with thumbs up emoji

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

github-actions bot commented Sep 23, 2023

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 bot added the blocked Waiting for another PR/issue to be merged/closed. label Sep 23, 2023
Copy link

github-actions bot commented Nov 24, 2023

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

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

Reviewers

No reviews

Assignees

No one assigned

Labels

blocked Waiting for another PR/issue to be merged/closed. stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants