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

Better parse error handling #7

Merged
merged 2 commits into from
Mar 6, 2023
Merged

Better parse error handling #7

merged 2 commits into from
Mar 6, 2023

Conversation

hhhjort
Copy link
Collaborator

@hhhjort hhhjort commented Jan 4, 2023

No description provided.

guscarreon
guscarreon previously approved these changes Jan 10, 2023
Copy link

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -13,3 +13,6 @@ const (
SectionUSPUT SectionID = 11
SectionUSPCT SectionID = 12
)

var SectionIDNames = []string{"ID0", "ID1", "tcfeu2", "gpp header", "ID4", "ID5", "uspv1", "uspnat",
"uspca", "uspva", "uspco", "usput", "uspct"}
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO: It's a bit odd to see names set for the value 0 which is undefined. There are names for sections 1, 4, and 5 - but they're not supported by this library. Based on how the function is used, it's save to ignore values for which are not parsed.

How do you feel about using a map for supported sections?

var sectionNamesByID = map[int]string{
  2: "tcfeu2",
  3: "gpp header",
  ...
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Works for me.

parse.go Outdated Show resolved Hide resolved
parse.go Outdated Show resolved Hide resolved
},
expectedError: []error{fmt.Errorf("error parsing GPP header, section identifiers: error reading an int offset value in a Range(Fibonacci) entry(1): error reading bit 4 of Integer(Fibonacci): expected 1 bit at bit 32, but the byte array was only 4 bytes long")},
},
"GPP-uspca-error": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice naming. 👍

Copy link

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

LGTM

@hhhjort hhhjort merged commit dfb6d3b into main Mar 6, 2023
@SyntaxNode SyntaxNode deleted the sectionError branch January 22, 2024 21:56
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