Skip to content

Comments

Implement customizable serialization, make vibe-d:data optional#59

Merged
Geod24 merged 1 commit intov0.x.xfrom
serialization
May 4, 2020
Merged

Implement customizable serialization, make vibe-d:data optional#59
Geod24 merged 1 commit intov0.x.xfrom
serialization

Conversation

@Geod24
Copy link
Owner

@Geod24 Geod24 commented Mar 19, 2020

This will be used by Agora, for types that are not JSON-serializable.

This is marked as a draft ATM, because it doesn't pass the tests with Agora.

@AndrejMitrovic
Copy link
Contributor

This is nice. What's the failure in Agora?

@Geod24
Copy link
Owner Author

Geod24 commented Apr 17, 2020

Memory corruption because things are moved

@Geod24 Geod24 force-pushed the serialization branch 2 times, most recently from 3ba11b0 to 0e932e2 Compare April 29, 2020 05:43
@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #59 into v0.x.x will increase coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x      #59      +/-   ##
==========================================
+ Coverage   93.08%   93.27%   +0.19%     
==========================================
  Files           3        4       +1     
  Lines         665      684      +19     
==========================================
+ Hits          619      638      +19     
  Misses         46       46              
Flag Coverage Δ
#unittests 93.27% <100.00%> (+0.19%) ⬆️
Impacted Files Coverage Δ
source/geod24/LocalRest.d 96.13% <100.00%> (+0.13%) ⬆️
source/geod24/Serialization.d 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65c1e2a...d9bd6e6. Read the comment docs.

@Geod24 Geod24 force-pushed the serialization branch 2 times, most recently from f427b9f to fd57529 Compare April 29, 2020 05:58
@Geod24 Geod24 marked this pull request as ready for review April 29, 2020 09:15
@Geod24
Copy link
Owner Author

Geod24 commented Apr 29, 2020

This is ready

@AndrejMitrovic
Copy link
Contributor

core.exception.AssertError@source/geod24/concurrency.d(1136): Error: MessageBox is closed

Strange random failure on ldc. I wonder if we have a bug somewhere..

@Geod24
Copy link
Owner Author

Geod24 commented Apr 29, 2020

Hum, I see it in Agora when I switch to the Serializer. Will look deeper into it.

Serialize arbitrary data to / from JSON

This is the default serializer used by LocalRest, when none is provided.
It is a template so that LocalRest does not import Vibe.d if not used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't it be imported regardless when -unittest is passed in the client code? Because the unittest block below will instantiate this template.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Unittests in non-root modules should not be parsed

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm.. this is when using dub only, right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No it's a DMD behavior, see discussions here: dlang/dmd#10937

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah interesting. I know Walter implemented the "don't parse unittest blocks when -unittest is not passed", but I didn't know about this other feature.

Copy link
Contributor

@AndrejMitrovic AndrejMitrovic left a comment

Choose a reason for hiding this comment

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

LGTM. Just had a few questions.

This will be used by Agora, for types that are not JSON-serializable.
@Geod24 Geod24 merged commit bd50c60 into v0.x.x May 4, 2020
@Geod24 Geod24 deleted the serialization branch May 4, 2020 01:20
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