-
Notifications
You must be signed in to change notification settings - Fork 6
Adding text for the json xtype #52
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
Conversation
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.
As usual the whitespace edits add some unhelpful noise.
DALI.tex
Outdated
\emph{point}, \emph{circle}, \emph{range}, \emph{polygon}, \emph{moc}, | ||
\emph{multipolygon}, \emph{shape}, \emph{uri}, and \emph{uuid} (see below). | ||
|
||
A special case is the \emph{json} xtype: I allows publishers to, in |
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.
s/I allows/it allows/
DALI.tex
Outdated
Libraries supporting the json xtype should return deserialised sequences | ||
and mappings as appropriate for the host language for json payloads. |
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 don't know whether this comment is necessary or beneficial. The other xtypes don't have anything similar, for instance timestamp doesn't say that libraries should return a language-specific timestamp object. I'd be inclined to delete this paragraph, but if not I would prefer "may" to "should".
On Tue, May 06, 2025 at 01:21:43AM -0700, Mark Taylor wrote:
@mbtaylor requested changes on this pull request.
As usual the whitespace edits add some unhelpful noise.
Aw, bother. Perhaps we ought to have a linter for ivoatex that
complains about (among many other things) trailing whitespace?
Anyway, I've concentrated the whitespace thing into a separate
commit. Perhaps we should merge this PR by rebasing for the sake of
(git) history.
Review
f143946
to have the whitespace changes weeded out.
> +Libraries supporting the json xtype should return deserialised sequences
+and mappings as appropriate for the host language for json payloads.
I don't know whether this comment is necessary or beneficial. The
other xtypes don't have anything similar, for instance timestamp
doesn't say that libraries should return a language-specific
timestamp object. I'd be inclined to delete this paragraph, but if
not I would prefer "may" to "should".
Hm. Do we have language that says that parsers should return
"native" values whenever they can? If so, I'm happy to drop this.
If not... well, if someone else also dislikes this, I'd still drop
it, but I, for one, would like it if there was clear guidance as to
what user code is entitled to expect in particular in this case.
Are you concerned that certain libraries might want to further
"distill" such columns and, for instance, return geometry objects if
they suspect something is, say, geojson? Or is it just the symmetry
with the other xtypes?
|
I just think it's a decision for the library to make whether it wants to package strings into JSON objects or vertex arrays into polygons or whatever, or to offer the user the lower-level string serialization that actually comes out of the VOTable. Maybe the user wants to see the lower-level string (e.g. for rewriting it to some other table format), or maybe the user does want a JSON object but using a different JSON library than that chosen by the VOTable library author. My approach writing a Java I/O library would probably be to provide the output as a string value, but provide or point to some additional function somewhere that's able to package that up as a JSON object or polygon or whatever if that seemed appropriate. Other library authors or other languages might have different approaches or expectations.
IMO such expectations don't belong in documentation of the serialization format. |
On Tue, May 06, 2025 at 05:08:51AM -0700, Mark Taylor wrote:
mbtaylor left a comment (ivoa-std/DALI#52)
> it, but I, for one, would like it if there was clear guidance as to
> what user code is entitled to expect in particular in this case.
IMO such expectations don't belong in documentation of the serialization format.
Hmmmm :-)
Don't count me as totally convinced, but I've dropped the paragraph.
Let's see what people will do.
|
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.
Looks fine to me now. If other reviewers prefer the recommendation about library behaviour to be left in, fair enough.
I agree with Mark's comments. I have fixed the conflicts caused by merging the other PR just now. |
DALI.tex
Outdated
(e.g., as a \xmlel{GROUP} for a \xmlel{PARAM} or as unrolled | ||
\xmlel{FIELD}\/s or a separate table for table cells) is preferred. | ||
|
||
\xmlel{PARAM}\/s and \xmlel{FIELD}\/s containing JSON values should be |
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.
This could probably be simplified and restated in the style I ended up using for the other xtypes. maybe:
JSON values serialised in VOTable or described in service parameters should have the following VOTable type metadata:
\begin{verbatim}
datatype="char" arraysize="*" xtype="uri"
\end{verbatim}
The general allowance for unicodeChar and more specific arraysize would apply as it does elsewhere.
DALI.tex
Outdated
the following VOTable type metadata: | ||
|
||
\begin{verbatim} | ||
datatype="char" arraysize="*" xtype="uri" |
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 should be xtype="json"
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.
yes, that was just a cut and paste of the style I found worked best
This is intended to address bug #33. See there for a discussion of why this text is fairly hedging.