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

Also allow data to have disabled copy constructor.

@s-ludwig
Copy link
Member

s-ludwig 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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

Copy link
Member

@s-ludwig s-ludwig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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