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

Made AreaId encodable and sortable #95

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stan-at-work
Copy link
Contributor

@stan-at-work stan-at-work commented Aug 6, 2024

This PR fixes this issue.

@stan-at-work
Copy link
Contributor Author

Any updates on this ?

@caduandrade
Copy link
Owner

Hi @stan-at-work!

A few points:

  1. I think _AreaId should not receive an id, because if there is an id, _AreaId would not even be instantiated. This class represents an automatically generated or undefined id. Perhaps the name could be _AreaUndefinedId.

  2. This change does not make Area encodable, only _AreaId. Right? I believe the Area will not be possible to do it automatically because it receives functions as parameters. In this case, perhaps the solution is to have a (static?) registry to map functions with some identifier. The toJson method could replace the function with its identifier. The fromJson method could obtain the correct function from the registry given the identifier. Anyone using the API could map their functions to this registry.

What do you think? Does that make sense?

@stan-at-work
Copy link
Contributor Author

Hey ✋

1: Yes and no... I like to give people the freedom to pass their own parameters to objects if possible.
I think it does no harm if you can pass them in. This way, we give others the freedom and are not limited to one implementation.

2: Correct, this only makes _AreaId encodable, not Area.
You can manually make Area encodable, so that's not necessary.

@stan-at-work
Copy link
Contributor Author

@caduandrade Hello ?

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.

Error Encoding Area Object to JSON
2 participants