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

Add HeaderSizeError for downstream consumers to match on #54

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

Daenyth
Copy link
Contributor

@Daenyth Daenyth commented Aug 10, 2020

When raising header size mismatches, use this specific error.

It will cause, for example, exception reporting software like Sentry to
correlate these exceptions separately from logical errors like a bad Decoder

This should be backwards compatible since it's a subclass

@satabin
Copy link
Member

satabin commented Aug 10, 2020

While we are at it, could it make sense to also have two extra fields actual and expected?

@Daenyth
Copy link
Contributor Author

Daenyth commented Aug 10, 2020

I thought about that, but it wasn't clear to me that it was useful beyond just knowing that it happened. But it's cheap to add, so sure.

When raising header size mismatches, use this specific error.

It will cause, for example, exception reporting software like Sentry to
correlate these exceptions separately from logical errors like a bad Decoder
@satabin satabin added csv enhancement New feature or request labels Aug 10, 2020
@satabin satabin added this to the 0.8.0 milestone Aug 10, 2020
@satabin satabin self-requested a review August 10, 2020 16:38
Copy link
Member

@satabin satabin left a comment

Choose a reason for hiding this comment

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

LGTM

@satabin satabin merged commit 9b2efcd into gnieh:master Aug 10, 2020
@Daenyth Daenyth deleted the gb-header-size branch October 14, 2020 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
csv enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants