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

Panics have changed with Errors for NewWriter And NewReader #246

Merged
merged 5 commits into from Apr 4, 2019

Conversation

olimpias
Copy link
Contributor

Errs are handled more gracefully.
Unit tests are added for NewWriter and NewReader.

Ref Task: #241

Unit tests are added for NewWriter and NewReader.
@achille-roussel achille-roussel self-assigned this Mar 31, 2019
@achille-roussel
Copy link
Contributor

achille-roussel commented Mar 31, 2019

First, thanks for the contribution and raising the issue!

I don't think we can make this change as proposed tho, because we would be breaking pretty much all programs using this package.

What do you think about taking a different approach, and introducing a mechanism for validating the configuration values that are passed to reader/writer, for example:

func (c *ReaderConfig) Validate() error {
    ...
}

Then the program can explicitly call Validate to detect malformed configuration before it creates the reader, and we can leave the signature of NewReader/NewWriter unchanged.

What do you think?

@olimpias
Copy link
Contributor Author

olimpias commented Mar 31, 2019

Sure, it makes sense for backward compatibility. I will do the required updates.

Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the contribution!

@achille-roussel achille-roussel merged commit cd74b6e into segmentio:master Apr 4, 2019
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.

None yet

2 participants