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

Modify code to allow :initform to be used for unspecified fields. #2

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

Conversation

smcallis
Copy link

When specifying a class slot, we're allowed to pass an :initform
specifying a form to use as the initial value of the slot when it's
not passed to the constructor.

Currently, if we we specify a slot with jeison-defclass, the behavior
is to require that field be present in the decoded json. This is a
problem for situations where fields may have a default value, but that
default isn't necessarily encoded in the payload (eg: gRPC). Thus we
change the default behavior to allow fields to not be present.

This is a two part fix:

  1. modify jeison--read-internal to allow null fields to skip type check
  2. modify jeison--read-slot to return nil when a slot value is null

Then, when jeison--read-class applies the constructor to initialization
values, the unspecified fields will be absent and the constructor will
use their :initform instead.

When specifying a class slot, we're allowed to pass an :initform
specifying a form to use as the initial value of the slot when it's
not passed to the constructor.

Currently, if we we specify a slot with jeison-defclass, the behavior
is to require that field be present in the decoded json. This is a
problem for situations where fields may have a default value, but that
default isn't necessarily encoded in the payload (eg: gRPC). Thus we
change the default behavior to allow fields to not be present.

This is a two part fix:
  1) modify jeison--read-internal to allow null fields to skip type check
  2) modify jeison--read-slot to return nil when a slot value is null

Then, when jeison--read-class applies the constructor to initialization
values, the unspecified fields will be absent and the constructor will
use their :initform instead.
@smcallis
Copy link
Author

smcallis commented May 6, 2022

Any thoughts on merging this?

@patrickt
Copy link

I ran into this and would love to see it fixed.

@smcallis
Copy link
Author

Sadly this repo's had no commits for three years so I fear the worst :(

@bscottm
Copy link

bscottm commented Feb 14, 2023

@patrickt, @smcallis: It does look like this particular repo has hit a dead end. I'm going to fork it -- I added support for a vector-of type specification (yeah, that's actually useful) and I've integrated this merge request. I'm not ready to publish my fork (and I haven't pushed my updates either). Prospectively, it'll be on bscottm/decljson in the next day or two.

Are there any other enhancements you'd like to see?

@smcallis
Copy link
Author

Nope mostly compatibility with gRPC was what I was after, so default values is all I need.

@bscottm
Copy link

bscottm commented Feb 15, 2023

@smcallis, @patrickt: Pushed updates/enhancements to my fork (bscottm/decljson). In addition to the :type (vector-of type) constraint, there's also :type (hashtab-of type) constraint, support both native and json.el parsers.

@SavchenkoValeriy
Copy link
Owner

Hi there folks, sorry I got out of my hibernation :)
@smcallis would you be able to add a couple of tests and a short description for the README?

As part of the consideration, maybe we can make this behaviour optional? Like mark a slot that it might be absent in the actual JSON and that some other value should be used in that situation.
It might come as a surprise for users who rely on stricter checks right?

@SavchenkoValeriy
Copy link
Owner

@bscottm Hey, I see you worked a bit on your own fork. Sorry for being so unresponsive.
Would you mind creating PRs with your changes?

@bscottm
Copy link

bscottm commented Feb 17, 2023

@SavchenkoValeriy: I incorporated this pull request into my fork, so you can effectively ignore this one.

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