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

Added proper handling of UTF-16 errors when parsing #53

Closed

Conversation

phillipCouto
Copy link
Collaborator

Created a ParseError enum to serve as the new home for parse errors. Added UTF-16 to the enum and updated the windows code to now use the new ParseError::FromUtf16 error.

Also added some comments, they may seem self explanatory but figured it is an easy quality of life add. I can remove if you want 😄

@phillipCouto
Copy link
Collaborator Author

@hwchen and @adobeDan this is an example of a breaking change just because we added to the enum. It would be great to know how we will deal with these as it would be best if we didn't have to hold changes back until we were ready to cut a new major release as that can get messy with conflicts if the activity of the project grows.

Added UTF parsing errors to enum
@hwchen
Copy link
Owner

hwchen commented Nov 13, 2021

For this change, I'd like to see how @adobeDan 's changes to error handling shake out first, this should be pretty easy to incorporate afterwards.

Using a struct for errors can reduce breaking changes (if I'm understanding correctly). Also, for just adding variants we can use something like #[non_exhaustive] which will prevent breakage for pattern matching.

@adobeDan
Copy link
Collaborator

@phillipCouto Can you please do a thorough review of #67 and #70 and let me know if you like this direction? If so I think we should do a major release as they will take care a lot of accumulated backlog and that way we will warn clients they need to make changes in order to adopt.

@brotskydotcom
Copy link
Collaborator

This PR's changes are now subsumed in #70, so I'm closing this PR.

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.

4 participants