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

minimally functioning sam parser #333

Closed
wants to merge 12 commits into from
Closed

minimally functioning sam parser #333

wants to merge 12 commits into from

Conversation

Koeng101
Copy link
Contributor

@Koeng101 Koeng101 commented Aug 17, 2023

This PR adds a sam file parser. Minimal functioning parser created, needs testing. Doing that next.

Not ready for merge.

@Koeng101 Koeng101 added the draft label Aug 31, 2023
Copy link
Collaborator

@carreter carreter left a comment

Choose a reason for hiding this comment

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

Oops, probably left more comments than I should have... 😅

io/sam/SAMv1.pdf Outdated Show resolved Hide resolved
io/sam/sam.go Show resolved Hide resolved
io/sam/sam.go Outdated Show resolved Hide resolved
io/sam/sam.go Outdated Show resolved Hide resolved
io/sam/sam.go Outdated Show resolved Hide resolved
io/sam/sam.go Outdated Show resolved Hide resolved
io/sam/sam.go Show resolved Hide resolved
io/sam/sam.go Outdated Show resolved Hide resolved
return Alignment{}, err
}
parser.line++
line := strings.TrimSpace(string(lineBytes))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, style: Same as above, disambiguate this from parser.line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does that mean exactly? What should be disambiguated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

parser.line refers to the line number, while line refers to the line contents. IMO would help readability to make the difference explicit in the names.

io/sam/sam.go Outdated
}

// ParseNext parsers the next read from a parser. Returns an error upon EOF.
func (parser *Parser) ParseNext() (Alignment, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some whitespace/comments to this to make it easier to read!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I definitely need to do that. Lil hard to read right now.

@carreter carreter removed the draft label Sep 23, 2023
@carreter
Copy link
Collaborator

What's the status of this? If it's ready for review, I'll take a look in the coming couple of days.

@Koeng101
Copy link
Contributor Author

Well I haven't updated it to merge into #339 , so a bit to go there. I think it basically works though.

@carreter
Copy link
Collaborator

Alright, will give it a looksie tomorrow then!

@Koeng101 Koeng101 added the blocked Waiting for another PR/issue to be merged/closed. label Nov 15, 2023
@Koeng101 Koeng101 closed this Dec 7, 2023
@Koeng101 Koeng101 deleted the samParser branch December 7, 2023 17:06
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants