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 struct to serializable-struct. #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LeifAndersen
Copy link
Contributor

This library is a graph library, the serialize library is designed
to serialize cyclic data structures. These two libraries should be
talking to each other.

This library is a graph library, the serialize library is designed
to serialize cyclic data structures. These two libraries should be
talking to each other.
@stchang stchang mentioned this pull request Jul 25, 2019
@mkohlhaas
Copy link
Contributor

I think persistence shouldn't be mixed into the existing graph structs in the sense of making it a default behavior. If one wants to have persistence on top of the existing structs one could define a persistent struct with define-serializable-struct (or something similar) and specify the existing graph struct as a super struct.

There could be new graph types that don't use a Racket struct at all but store the graph in e.g. files, databases or any kind of external entities.

@stchang
Copy link
Owner

stchang commented Apr 6, 2020

I agree (which is probably why I never merged this ticket). Given the generic implementation, a representation using serializable structs should be straightforward to add. This way, existing code is unaffected.

@LeifAndersen
Copy link
Contributor Author

To clarify: I mostly was just saying the built in graph implementations in this library can be serializable with no downside. Where as not doing it requires a bunch of redundant work for any end users who want to serialize their graphs. This patch will not break any code, unless they were relying on the serialize? functions for the default graphs Stephen made to return #f. Something like:

(define graph (make-directed-graph '()))
(if (serializable? graph)
    <<launch-the-missles>>
    <<something-else>>)

@mkohlhaas I presume by persistence you are actually meaning serializability? I ask for clarification as serializability and persistence are fairly unrelated.

@stephen and @mkohlhaas Anyway, in principle, yes, anyone could construct a new struct type and make them serializable and also meet the graph API. But that would require them to implement 20 or so methods. Which just seems silly given that the implementations would be exactly the same as the default implementations built into this graph. It would also make for a bad pull request.

@bennn
Copy link

bennn commented Apr 6, 2020

I agree with Leif, serializability would be a very useful default and seems unlikely to break things.

@mkohlhaas
Copy link
Contributor

Hi Leif,

yes that's true. I mean serialization for the purpose of persistence. Would you use it for something else?

I thought one could create a new serializable struct referring to an existing structure without reimplementing all the methods. Cf maybe-super.

It's awesome having a discussion with all you Racket masters I know from YouTube! 😀
I should go to the next Racket conf and make some selfies with you guys.

Sidenote: I'm pretty new to Racket. No way I could keep up with you guys. I'm just divulging my naive opinions.

@stchang
Copy link
Owner

stchang commented Apr 6, 2020

@mkohlhaas Thanks for resuming this conversation.

@bennn Did you have a particular use case in mind? Do other data structures use serializable structs?

@LeifAndersen Did you initially create this PR for the feature-specific profiler?

@bennn
Copy link

bennn commented Apr 7, 2020

No, I don't have a need for this right now. But it would be useful the same way that serializing lists & hashes can be useful.

If graphs were serializable, then I could have used them instead of adjacency lists to represent module-dependence graphs for the gradual typing benchmarks (link).

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

4 participants