-
Notifications
You must be signed in to change notification settings - Fork 4
Add a forMode attribute to several elements #10
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
base: master
Are you sure you want to change the base?
Conversation
728b476
to
cc1c6b0
Compare
Yes, I think this would be a good solution. As I noted in my comments on #8 there is some scope for older clients getting confused by multiple limit declarations differentiated by an attribute they don't know about. In existing versions of TOPCAT, I think this would result in multiple apparently contradictory entries in the TAP window Max Rows selector (only really apparent if you actually click on the thing, which probably most users don't most of the time). That could look a bit weird to users, but I wouldn't say it's catastrophic, and arguably is more informative than the existing state where you see a single limit that may be invalid for one of sync/async. I can't speak for other clients. |
I think this approach looks great and does indeed seem to work for to the various use-cases. Should we clarify how clients should choose if both a general declaration and a mode-specific declaration exist for the same element type? For example say that the service declares:
What would be the default for sync? I'm assuming it sould it be 60, but is this clear what the clients should do based on the document? And is the hard-limit for sync 120 or undefined? In other words should clients scan all elements and merge them, or use first-match? The second edge case is that the schema allows multiple declarations for the same mode.
Should we outline how clients should handle this, and should the document prohibit this somehow? Something like:
If the capabilities document violates this constraint, clients SHOULD/MUST(?) use the first matching element. |
My understanding for this example is the following for sync mode: default=60 and max=120. I agree this corner case should be explicitly described. I'd say that the default block (i.e. the one with no |
Here, I'd say there are two approaches:
My preference goes for the 2nd option, as @stvoutsin suggests, as the 1st one will be clearly more confusing for everyone. If this one is adopted, may I suggest to reflect this in the XML schema by setting the attribute |
I'd say that the text in the proposed section 2.2 Mode-dependent Declarations covers this in sufficient detail:
Admittedly it doesn't explicitly forbid adding multiple contradictory limits; that could be added but it seems pretty obvious to me how that would work (services shouldn't do it, I don't know why they would, and if clients find such things they'd be within their rights to use whichever one they like). |
My bad, I did not read the diff of the .tex part...sorry. Indeed the 1st corner case of @stvoutsin seems to be covered there. But as you said, not the 2nd one. I'd say it should because of the behavior confusion that it would bring, on both server and client sides. |
I'm happy with the revised text. |
It looks good to me too. |
On Thu, Oct 02, 2025 at 02:30:48AM -0700, Grégory Mantelet wrote:
gmantele left a comment (ivoa-std/TAPRegExt#10)
But as you said, not the 2nd one. I'd say it should because of the
behavior confusion that it would bring, on both server and client
sides.
Hm... regrettably, I have a bit of a hard time to put this into
words in an understandable way. The main trouble is that multiple,
say, outputFormat elements with the same forMode are ok, but for
limits we definitely don't want that. Saying that makes me realise
that my current recipe in the section on Mode-dependent Declarations
doesn't work.
I'm trying again in commit #7165975 and immediately dislike the
language. That this is so complicated is instilling doubts in me
whether that's actually what we should do. Hm.
Better text solicited, either way.
|
I think you're being too hard on your text. It looks comprehensible to me. |
This version looks good to me, but I still wonder if there might be ambiguity in the edge case that I mentioned. The text specifies that
However override could potentially be interpreted as a complete replacement, or a merge/patch. Also one thought is should we change the text structure a bit based on the two distinctive behaviours?
Having said that if you disagree that the edge case is ambiguous or don't like this structure, what you have is also perfectly fine from my point of view so I'm happy if you want to merge either way. |
On Fri, Oct 03, 2025 at 08:50:29AM -0700, stvoutsin wrote:
stvoutsin left a comment (ivoa-std/TAPRegExt#10)
However override could potentially be interpreted as a complete replacement, or a merge/patch.
My interpretation would be a complete replacement because we say
"limits given in elements" override rather than "child values in
elements", which suggests to me the entire element is the unit of
replacement.
The intention definitely is merge; if you only override a hard limit,
the default limit would stay in place.
Also one thought is should we change the text structure a bit based on the two distinctive behaviours?
I'm thinking something like this:
That is probably clearer, yes. Would you care to prepare a PR with
that text? That would be wonderful...
|
This is an alternative to PR #8 and was developed in the discussion there.