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

feat: optional to/from methods on Reflector. (#150) #152

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

m-mcgowan
Copy link
Contributor

Changes:

  • Separates the has_reflector concept into has_read_reflector and has_write_reflector to allow allow Reflector types to be just readable, just writable, or both readable and writable.

  • Adds test cases. The tests are slightly altered from the original test_reflector implementation. They now use a small class hierarchy (Person and Parent) with fields in each class, which is not supported by default. The Reflector implementation in the tests flattens the hierarchy. Test cases for Reflector implementations with only the to and only the from method are present.

  • Adds a paragraph to the docs mentioning the user-facing changes.

Fixes issue: #150

@liuzicheng1987
Copy link
Contributor

Looks good to me. I will have to wait for the code checks to run through, but just by looking at the code changes, it looks fine. Thank you so much for implementing this so quickly.

Just one minor thing: In your write_and_read.hpp, you still have this #if 0 ... #endif section, which appears to be something you meant to remove, but then forgot. Could you just quickly remove this?

@m-mcgowan
Copy link
Contributor Author

Thanks for catching that. I wanted to implement write_and_read in terms of read and write, but after a few minutes I gave up :-)

Unused code removed.

@liuzicheng1987 liuzicheng1987 merged commit 0045030 into getml:main Jul 17, 2024
7 checks passed
@liuzicheng1987
Copy link
Contributor

All right, merged. Thank you for your contribution!

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