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] Add message parsing #2

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

[WIP] Add message parsing #2

wants to merge 3 commits into from

Conversation

jimeh
Copy link
Member

@jimeh jimeh commented Aug 15, 2021

This is nearly a complete rewrite of #1 from last weekend :P

I opted for a slightly different approach to breaking down and accessing data out of the a commit message, which I believe is a bit more performant, and yields less and simpler code further down the parsing chain.

Main message parsing does seem to work, but more test cases are needed, and some cleanup too I'm sure.

The old RawMessage implementation effectively brute forced the initial
processing of a comment message by breaking it down into lines, and
grouping them into paragraphs.

This is useful, but, we actually only need the first paragraph, the last
paragraph, and then everything between. So there's no need to break down
the message into each paragraph.

In theory, the Buffer implementation is more performant than RawMessage
was, but most importantly I think it will be easier to work with it.
@jimeh jimeh requested a review from geoah August 15, 2021 20:43
Copy link
Member Author

@jimeh jimeh left a comment

Choose a reason for hiding this comment

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

Well, this seems to be what happens when I can't sleep. I end up messing around with the GitHub iPadOS app and review one of my own PRs as a form of note keeping about what to fix, cause getting my laptop out and just doing the fixes is more effort than I'm willing to commit to at 1am... lol

message.go Outdated Show resolved Hide resolved
buffer.go Outdated Show resolved Hide resolved
line.go Outdated Show resolved Hide resolved
buffer.go Show resolved Hide resolved
buffer.go Show resolved Hide resolved
//
// It is intentionally VERY forgiving so as to be able to extract the various
// parts even when things aren't quite right.
var HeaderToken = regexp.MustCompile(
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be better to expose helper functions for checking if a string/[]byte matches a header/footer token, and also functions that extract the component parts in a standardized way.

Otherwise the capture groups of these regexp patterns effectively become part of the package's public API, making it difficult to change. Hence this public interface should be provided as helper functions instead, meaning the underlying implementation can be safely fixed and/or improved without breaking backwards compatibility.

@codeclimate
Copy link

codeclimate bot commented Aug 26, 2021

Code Climate has analyzed commit bf44c4a and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

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.

1 participant