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

change error message #97

Merged
merged 3 commits into from Jan 31, 2017
Merged

change error message #97

merged 3 commits into from Jan 31, 2017

Conversation

@TyOverby
Copy link
Collaborator

TyOverby commented Jan 31, 2017

I didn't like the old error message

@TyOverby
Copy link
Collaborator Author

TyOverby commented Jan 31, 2017

@dtolnay
Copy link
Collaborator

dtolnay commented Jan 31, 2017

Seems reasonable. Another option would be to add a SerializeError variant for this case.

@TyOverby
Copy link
Collaborator Author

TyOverby commented Jan 31, 2017

I like that more actually.

@dtolnay
Copy link
Collaborator

dtolnay commented Jan 31, 2017

I don't think "infinite" is the right word, especially because SizeLimit::Infinite means something completely different. If serialize_seq does not receive a size, it is because the Serialize type does not know the length up front, for example because we are serializing an iterator that can only be iterated once.

@@ -49,6 +49,7 @@ impl fmt::Display for SerializeError {
match *self {
SerializeError::IoError(ref err) => write!(f, "IoError: {}", err),
SerializeError::Custom(ref s) => write!(f, "Custom Error {}", s),
SerializeError::InfiniteSequence => write!(f, "InfiniteSequence"),

This comment has been minimized.

@dtolnay

dtolnay Jan 31, 2017

Collaborator

Please make this a human-readable error message. This is the Display impl not Debug.

@TyOverby
Copy link
Collaborator Author

TyOverby commented Jan 31, 2017

@dtolnay Changed it to SequenceMustHaveLength and the Display impl mentions maps and seqs

@dtolnay
Copy link
Collaborator

dtolnay commented Jan 31, 2017

Cool. I filed #98 to follow up on the other messages.

@TyOverby TyOverby merged commit 6856732 into master Jan 31, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@TyOverby TyOverby deleted the change-seq-message branch Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.