Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
JSON support #51
JSON support #51
Changes from all commits
4e43e63
f611179
e3d8e83
c5394f7
72708eb
2a0c713
593b32e
4c50ce6
0198334
a827a77
8af0e54
32068c0
5ffcf35
6ba1735
74f9b0b
a0a8166
c032f8b
2de6306
92a8f8d
390e288
dcd06cc
f23ed82
efc4b0b
d7064d2
4e6b74a
d75efc1
ddea84f
92dcba1
515c6fb
95462d2
1670c1d
b448fde
cdb9fd0
3c71738
d1bc8dc
634f88c
08cf34d
26af65f
578cb95
b96bbae
eb41584
899fcb7
9a321aa
06dd2c6
22f2258
829ae34
8c7efb8
9b4bd4a
7fb9e02
a1cae95
5553fb8
7723fb5
5d196cd
e0827c5
942be81
32bddbe
2029a96
c984b22
bbf2891
99d189b
dc50674
0a8acf7
d36231a
43e51b1
989c6d9
407432f
ff5ce0b
0ea991d
99f0662
fb26d7f
10545c8
e84b10a
53ce3dd
6136cda
4aacbd6
7d626c5
c35669c
4287c0b
79551df
8992608
f277ffe
d2fa10f
acdb6ec
ceed951
fc2ee8c
53b7abf
2753e76
35cca47
62d5c89
da1b5ac
45055e1
a58e01d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be some redundancy with the PR with the Heap. Could you synchronize with @prvshah51 to ensure we define these constant in only one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Parva didn't include these in the end; right? So no conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we could automatically optimize
Option<byte>
to this in code generation instead, and the snake casing also feels wrong for a type definition in Dafny. I think I'd be happier with keeping this internal to the JSON code if possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's think about names for a moment. Names are the crown of all the hard work you have been putting into this library, and now you have time to step back and think about it ^^.
Everything in library is an API, so using API as a name makes little sense. What about just
JSON
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot use just
JSON
, unfortunately :( That's because definitions in the JSON module cannot use definitions in submodules of it, likeJSON.XYZ
.I considered having two top-level modules JSON and JSONImpl, but that's not great either.
Finally, this respects the link between module names and file names (
JSON.API
inJSON/Api
).We ought to improve this in the new module system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping JSON.API, it would be possible to create a file in the folder above JSON.dfy that refines this module, so that users don't need to write JSON.API? Could you do this? I don't see myself writing JSON.API to invoke JSON routines. JavaScript offers JSON.stringify and JSON.parse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would introduce a mismatch between the file name and the file path. You can, however, do
import JSON.API as JS
, which is good, I think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good, but not good enough for what we want to be a default library. I'll take a pass on that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed this is good enough given the whole repo reserves the right to rename things for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.