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 data serialization methods to `auto ref in` #2275

Merged
merged 5 commits into from Jun 29, 2019

Conversation

@tchaloupka
Copy link
Contributor

commented Mar 2, 2019

Also allow data to have disabled copy constructor.

tchaloupka added 2 commits Mar 2, 2019
Also allow data to have disabled copy constructor.
@s-ludwig

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Nice that this works now for all supported compilers, this should definitely work in some way. Unfortunately, requiring getters and toXXX to be const, this is a hard breaking change. Wouldn't just auto ref without the in work, too (letting T be inferred as const)?

data/vibe/data/json.d Outdated Show resolved Hide resolved
@tchaloupka

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2019

Agreed would definitely break something.
I've lessened it just to auto ref and moved type qualifier awareness to serializers to handle as required.
It seems ok, but I'm not so sure all the cases are handled properly as it's a bit magic sometimes :)

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Thanks, this looks good now. The only thing I'm still wondering is how the correct isXSerializable traits should look like. I'm thinking about something like this, because the trait should only yield true for const(T) when it actually has a const toX method:

enum isXSerializable(T) = is(typeof(T.init.toX()) : X) && is(T.fromX(X.init) : T);
@tchaloupka

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

I've made the change, hopefully in the expected way :) Tests passed ok for me.

Copy link
Member

left a comment

Looks good as far as I can tell. Thanks and sorry for losing track of this PR until now.

@s-ludwig s-ludwig merged commit c6f00ce into vibe-d:master Jun 29, 2019
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing 419ec62...0d0ba12
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.