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

Create DataWriter as a base class for JSONWriter #16

Merged
merged 5 commits into from
May 3, 2017

Conversation

andrea-prearo
Copy link
Contributor

This PR addresses Issue #15 and the the additional feedback for PR #10.

Changes:
- Add HeaderDoc comments for JSONWriter and JSONWriterError.
Changes:
- Revert to using CLUWritable protocol instead of JSONWriter for
  better flexibility.
Changes:
- Rename JSONWriterError to DataWriterError.
- Refactor DataWriterError to handle all possible errors for
  data writer classes.
- Rename JSONWriterErrorTests to DataWriterErrorTests.
- Refactor writer error tests.
Changes:
- Add DataWriter class to encapsulate the details of writing data
  to a stream.
- Refactor JSONWriter to be a subclass of DataWriter.
Changes:
- Alphabetize error cases for DataWriterError.
@andrea-prearo
Copy link
Contributor Author

Hey @Geek-1001, I was thinking about tackling Issue #13 next. Unless there is some item with an higher priority I should look into. Thanks!

@Geek-1001
Copy link
Owner

Hi @andrea-prearo
Everything is great, thanks for your contribution! 🤘
I'm merging this PR.

I was thinking about tackling Issue #13 next.

Yes, sure, feel free to fix #13
Also, I'll do VideoWriter migration #11 today/tomorrow and create a PR so we can discuss my changes as well.

@Geek-1001 Geek-1001 merged commit a799eaf into Geek-1001:develop May 3, 2017
@andrea-prearo
Copy link
Contributor Author

Thanks @Geek-1001!

Your plan sounds good to me 👍

@andrea-prearo andrea-prearo deleted the data-writer branch May 7, 2017 03:06
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.

2 participants