-
Notifications
You must be signed in to change notification settings - Fork 6
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
rewrite of service descriptor section to clarify; resolve #78 and #80 #83
Conversation
also added small comment on VOSI endpoints related to WD-DALI-1.2
Wow James - either you are a fast reader or I can't type :-) |
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.
(a) On the VOSI change, I'm fine.
(b) "In addition, the PARAM element allows for additional child elements
(such as MIN, MAX, or OPTIONS) to convey sensible constraints on
possible values." is IMHO much too weak. My experience is that in
typical scenarios (even just specifying a cutout) the problem of empty
input boxes is still worse in datalink than with normal discovery
services; for standard parameters, they really depend strongly on the
dataset, for custom parameters, well, it's anyone's guess what ought to
come in.
Hence, I'd say this should read: "To allow for expressive, usable user
interfaces, operators SHOULD indicate useful ranges of parameters in MIN
and MAX children or, for enumerated parameters, indicate the valid
values in OPTIONS." Or something like that.
(c) I liked "The PARAM's description" (in line 899) better than the
PARAM description".
(d) Please revert the Makefile change (except perhaps the change to the
DOCDATE) -- VECTORFIGURES isn't really for SVGs, as browsers can
directly display them; if there'll be more of those (before LaTeX learns
SVG), we'll have to think about how to deal with them more generally.
Meanwhile, SOURCES (which is LaTeX's dependencies) really needs to depend
on role_diagram.pdf, as that needs to be built before LaTeX runs. And
the PDF must not be in VECTORFIGURES, as that's files that will be
converted to PNG for the HTML rendering, and we specifically don't want
that for the SVG.
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.
Thanks Pat, this is a significant improvement in comprehensibility. I have made some specific suggestions for change but I think it's close to ready.
On Mon, Nov 15, 2021 at 02:43:03AM -0800, Mark Taylor wrote:
@mbtaylor commented on this pull request.
> +For params with a fixed value (e.g. \attval{fly}{true}), the client {\bf must}
+treat it as a required parameter and include it in the service invocation; this allows
It's not quite how I interpreted it up till now; the supplied value
FWIW, Pat's interpretation is what I've implemented in my XSLT, and
it is what DaCHS expects, so I'd be all for keeping the normative
content as proposed by Pat.
|
I have made some tweaks (included Markus' suggestion about input param info, made intro to service descriptors more general, etc). Everything but the "fixed value" bit.... This was never well specified and your interpretation is consistent with normal VOTable usage. When writing this, I was trying to figure out if we could use The way I see it, it is going to be quite tricky to differentiate a PARAM with value="something" as "fixed" vs "default". The use case for the former is something like a dataset-specific descriptor that is fixed to a specific dataset with
where various bits of the other info (like MIN/MAX of some other param) could be depending on that fixed value. On the default side is something like
If the latter included a child OPTIONS with the other possible values, that could work and be interpretted as a "default". This is also why the language in "Clients should assume that these user-specified parameters are optional, but ..." comes in: without default values the client does have to specify something to drive the service. So, yes, it would be nice to be able to say whether value="foo" is fixed or a sensible default. Would something like this work? "a PARAM with a value: the value is a default value if there are child elements indicating other possible values and is a fixed value if there are not" It seems open to misinterpretation. Thoughts? Better Wording? Leave it at "fixed"? Counter example to the must(s):
Looks like a fixed param but ignoring it is simply "do not track" :-) |
On Mon, Nov 15, 2021 at 11:28:14AM -0800, Patrick Dowler wrote:
"a PARAM with a value: the value is a default value if there are
child elements indicating other possible values and is a fixed
value if there are not"
It seems open to misinterpretation. Thoughts? Better Wording? Leave
it at "fixed"?
I'm all for leaving it as fixed. We need some advanced metadata for
parameters anyway, at the very least to indicate whether they're
required, and whatever we come up with there can easily accomodate
the default value, too.
So, I'd say forget about defaults in this version (we could live
without them for a couple of years already) and get serious about
prototyping solutions for bug #51 (me, I've already put in LINK-type
annotation into DaCHS and https://github.com/msdemlei/datalink-xslt,
but I'm happy to change that into anything else people are willing to
consume).
|
Just to clarify for me and maybe others. What do you mean by bug in #51, Markus ? The optional versus mandatory PARAM status ? Was that really a bug or a lack of solution ? |
I also agree with letting that as a fixed value for the moment. For client developpers defaults can be inferred from the context sometimes (the way it works in Aladin at least) |
This was discussed in the past. In case you come from ObsCore, SSA, SIA and the service is SODA or SODA-like the values can be inferred from the dataset metadata. In general we don't need to repeat that in VALUES. So I suggest to add these words to Markus sentence after "OPTIONS" : , "in case these values cannot be inferred from relevant metadata retrieved before the service descriptor discovery". |
Current text looks good to me now, thanks for changes. |
One little suggestion in this part of the text adding "utype" this way Rationale for that is that for example SODA parameters have obvious matching with ObsCore FIELDS. they are actually forcing ObsCORE Fields of the cutout. May have other examples. (see SODA for diuscussion) |
merging blocked, maybe because Markus requested changes and accepted in comments rather than another review. I will dismiss that review since Mark approved and see what happens (github is wierd sometimes) Also moving Francois' last item about utypes to an issue; I think it needs use cases and discussion. |
On Tue, Nov 16, 2021 at 12:25:13AM -0800, François Bonnarel wrote:
> ...... and get serious about prototyping solutions for bug #51
> (me, I've already put in LINK-type annotation into DaCHS and
> https://github.com/msdemlei/datalink-xslt, but I'm happy to
> change that into anything else people are willing to consume).
Just to clarify for me and maybe others. What do you mean by bug in
#51, Markus ? The optional versus mandatory PARAM status ? Was that
really a bug or a lack of solution ?
Ah, I meant, "issue #51", i.e., the lack of a defined mechanism to
say whether a parameters is optional or mandatory.
|
Le 17/11/2021 à 08:45, msdemlei a écrit :
On Tue, Nov 16, 2021 at 12:25:13AM -0800, François Bonnarel wrote:
>
> > ...... and get serious about prototyping solutions for bug #51
> > (me, I've already put in LINK-type annotation into DaCHS and
> > https://github.com/msdemlei/datalink-xslt, but I'm happy to
> > change that into anything else people are willing to consume).
> Just to clarify for me and maybe others. What do you mean by bug in
> #51, Markus ? The optional versus mandatory PARAM status ? Was that
> really a bug or a lack of solution ?
Ah, I meant, "issue #51", i.e., the lack of a defined mechanism to
say whether a parameters is optional or mandatory.
Thanks Markus
…
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#83 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMP5LTAREAZ54GV3DFYOPVLUMNMSVANCNFSM5IALYTXQ>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
also added small comment on VOSI endpoints related to WD-DALI-1.2