Skip to content

Conversation

@lmichel
Copy link
Collaborator

@lmichel lmichel commented Dec 19, 2022

More flexible location of the MIVOT block.
It must enclosed in a RESOURCE@type=meta which must be enclosed in another RESOURCE.

mcdittmar
mcdittmar previously approved these changes Dec 19, 2022
Copy link
Collaborator

@mcdittmar mcdittmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@msdemlei
Copy link
Contributor

Hm – why do you keep the RESOURCE[type="meta"] wrapper at all? IIRC its only purpose was to not require changes to the VOTable schema when there was a single VO-DML block somewhere near the top of VOTABLE.

Now that VODML is a child of RESOURCE anyway: Why not write

...

directly? It's schema-ok, simpler, and gives VO-DML a more predictable location by schema (you can mix RESOURCE-s and TABLE-s, but foreign elements are always at the end).

Other than that, I'd use the opportunity to drop the line starting with "This extra feature" -- sure, it's always nice to motivate designs, but I think the "don't touch VOTable" thing is already mentioned in the introduction.

@Bonnarel
Copy link
Contributor

Bonnarel commented Dec 20, 2022 via email

@Bonnarel
Copy link
Contributor

Bonnarel commented Dec 20, 2022 via email

remove duplicated MODEL
@lmichel
Copy link
Collaborator Author

lmichel commented Dec 20, 2022

The content of RESOURCE@type=meta is meant to be descriptive only (section 3.4).
Screenshot 2022-12-20 at 16 38 22

Should we consider the MIVOT block as being descriptive only? I would say yes since it provides a model view on the data. This is certainly questionable however.
We can drop the type=meta requirement if this bothers the implementer job.

The other suggestion is to suppress the requirement for the MIVOT block to be into a specific RESOURCE.
This is feasible but one argument for this MR was not to force the MIVOT block to be in a mandatory location (the head of the host resource). If we drop the encompassing RESOURCE for MIVOT, we require the mapping block to be at the bottom of the host resource: Its location is forced again and therefore, we lose the flexibility we got from the actual MR.

I propose to merge the MR as it is in order to provide an uptodate document for the reviewers and to let both questions:

  • encompassing RESOURCE or not open
  • type=meta or not
    for a future PR

@lmichel lmichel merged commit c61aa80 into master Dec 20, 2022
@lmichel lmichel deleted the update/mivot_resource_location branch April 26, 2023 11:59
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.

5 participants