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

Feature/21/serialize all #32

Closed

Conversation

azriel91
Copy link
Contributor

@azriel91 azriel91 commented Sep 18, 2018

Closes #21.

Added a changelog as well, since I find that useful

@Peternator7
Copy link
Owner

Thanks for taking this!! I'll try and get it reviewed by this weekend :)

@Peternator7
Copy link
Owner

Both your PR's look good. :) Thanks for proactively helping out with this!! I appreciate it a lot. I think I'll squash this then merge your second PR. Does that sound good to you?

@azriel91
Copy link
Contributor Author

azriel91 commented Sep 23, 2018

That does sound good 🙂, but before that I'd like to do a few more things:

  • Add impl From<MyEnum> for MyEnumDiscriminants
  • Add impl<'_enum> From<&'_enum MyEnum> for MyEnumDiscriminants
  • Fix a typo that I just noticed: "followign"

shall do that now

@azriel91
Copy link
Contributor Author

azriel91 commented Sep 24, 2018

okay cool all done (including docs for From)! Not sure which level of squashing you prefer (like, squash both branches or each one individually), but squash away!

@Peternator7
Copy link
Owner

I completed the 2nd PR because I was worried about conflicts, and I think it had all your changes. If that's correct, let's go ahead and close this PR. Otherwise let me know!

@azriel91
Copy link
Contributor Author

Yeap it did, thank you! hi5 ✋

@azriel91 azriel91 closed this Sep 26, 2018
@azriel91 azriel91 deleted the feature/21/serialize-all branch September 26, 2018 19:32
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