Skip to content

Conversation

msdemlei
Copy link
Contributor

@msdemlei msdemlei commented May 5, 2025

This is intended to address bug #33. See there for a discussion of why this text is fairly hedging.

@msdemlei msdemlei mentioned this pull request May 5, 2025
Copy link
Member

@mbtaylor mbtaylor left a 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
Copy link
Member

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
Comment on lines 984 to 985
Libraries supporting the json xtype should return deserialised sequences
and mappings as appropriate for the host language for json payloads.
Copy link
Member

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".

@msdemlei
Copy link
Contributor Author

msdemlei commented May 6, 2025 via email

@mbtaylor
Copy link
Member

mbtaylor commented May 6, 2025

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.

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.

@msdemlei
Copy link
Contributor Author

msdemlei commented May 7, 2025 via email

mbtaylor
mbtaylor previously approved these changes May 7, 2025
Copy link
Member

@mbtaylor mbtaylor left a 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.

@pdowler
Copy link
Collaborator

pdowler commented May 7, 2025

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
Copy link
Collaborator

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"
Copy link
Member

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"

Copy link
Collaborator

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

@pdowler pdowler merged commit 29a8995 into ivoa-std:main May 8, 2025
1 check passed
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.

3 participants