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

refactor: Simplify getMessages and extractComment #11

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

alexandear
Copy link

@alexandear alexandear commented Nov 29, 2024

The PR simplifies implementation of getMessages by replacing bufio.Reader with bufio.Scanner for comment processing. Additionally, leverages strings.TrimPrefix to simplify extractComment.

@alexandear alexandear force-pushed the refactor/simplify-get-messages branch from 2b22fea to 72e9f8a Compare December 1, 2024 13:52
@alexandear alexandear force-pushed the refactor/simplify-get-messages branch from 72e9f8a to 661ad15 Compare December 1, 2024 13:54
@matoous matoous merged commit 94d1edd into matoous:master Dec 2, 2024
3 checks passed
@alexandear alexandear deleted the refactor/simplify-get-messages branch December 2, 2024 17:19
@ldez
Copy link

ldez commented Dec 26, 2024

There is a missing piece: when using a bufio.Scanner, you must check the error (Scanner.Err()).

The previous element was buggy too because the default size of the bufio.NewReader is 4096.

Reader.ReadLine() returns 3 elements: the read section (a line or less), a "prefix" status (to say if the line is fully read), and an error.
When using Reader.ReadLine() you must check the "prefix" to be sure there are no missing elements on the line.

The bufio.Scanner has a size limitation too (64 * 1024).

https://go.dev/play/p/3iSeXkgYTGt

The most "secure" way to read a file is bufio.NewReader but you need to check the "prefix".
In your context, bufio.Scanner can work but the error must be checked.

@alexandear alexandear mentioned this pull request Dec 26, 2024
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.

3 participants