search

open-contracting / ocdskit

A suite of command-line tools for working with OCDS data
https://ocdskit.readthedocs.io
BSD 3-Clause "New" or "Revised" License
17 stars 6 forks source link

mapping-sheet: Extension field is blank for extension fields added using refs #153

Closed romifz closed 3 years ago

romifz commented 3 years ago

This happens when using --extension combined with --extension-field.

I'm not sure if there is a use case in which we don't want to add the extension field for fields added using refs, but for the mapping-sheet case, I think it's confusing to see how some extension fields have a value and others don't.

jpmckinney commented 3 years ago

Can you give a specific example?

romifz commented 3 years ago

Sure.

In the file generated by the command:

$ ocdskit mapping-sheet release-schema-1.1.5.json --extension https://raw.githubusercontent.com/open-contracting-extensions/ocds_bid_extension/v1.1.4/ --extension-field extension > mapping-sheet.csv

One of the rows for bids/details/value has the value "Bid statistics and details" in the extension column, but the column is blank for the rows bids/details/value/amount and bids/details/value/amount. The same happens for bids/details/documents/.

In contrast, the fields under bids/statistics have the "Bid statistics and details" value in the extension column.

jpmckinney commented 3 years ago

Aha. It depends on what we want the behavior to be. The extension column is defined as "The name of the extension in which the field was defined". Since the Value and Document objects are defined by OCDS, the extension column is blank. If an extension had added a field to the Value object, then the extension column would have the name of that extension.

So, the current code correctly answers the question of "which extension defined this specific field". However, if we want to answer the question "which extension caused this JSON path to exist", then we will need different code (or the user will just have to get used to looking at the parent path, to see where it was defined).

I don't have a strong opinion on which use case to support, but we should check with other analysts before changing the behavior.

jpmckinney commented 3 years ago

Also, in terms of having two rows for bids/details/value: That was behavior from a long time ago, that I maintained in OCDS Kit. I think @duncandewhurst had a need for having two rows in the case of $ref or array fields, but I don't remember why.

romifz commented 3 years ago

I see. I'm using the mapping-sheet method to replicate the 'OCDS Extension Schemas' tab of the OCDS mapping template.

It would be nice then to have another command, or an option in mapping-sheet, to answer the "which extension caused this JSON path to exist" then. BTW I have no issues with duplicating rows for refs, I just mentioned that since only one of them has the extension name added.

duncandewhurst commented 3 years ago

Also, in terms of having two rows for bids/details/value: That was behavior from a long time ago, that I maintained in OCDS Kit. I think @duncandewhurst had a need for having two rows in the case of $ref or array fields, but I don't remember why.

The general reason was to preserve the titles and descriptions from the reference and the referenced object. The specific reason for using two rows to do that was so that the first row (for the reference) could be col-spanned as a header to distinguish arrays in the mapping template.

jpmckinney commented 3 years ago

The general reason was to preserve the titles and descriptions from the reference and the referenced object. The specific reason for using two rows to do that was so that the first row (for the reference) could be col-spanned as a header to distinguish arrays in the mapping template.

Thanks! I've added comments to the code.

jpmckinney commented 3 years ago

@yolile @duncandewhurst @pindec Presently, the behavior of the extension column in the mapping sheet is to indicate the extension that defines the field, not the extension that causes the JSON path to exist.

For example, the bid extension refers to the OCDS Value object. Presently, the first bids/details/value row mentions the bids extension, but the bids/details/value/amount, etc. rows don't (because they are defined by OCDS, not the extension). However, those full JSON paths only exist because of the bids extension.

Does anyone have an objection to changing the behavior so that the extension column indicates the extension that causes the JSON path to exist?

jpmckinney commented 3 years ago

@romifz You can use the branch in #154 until we decide on the behavior.

romifz commented 3 years ago

@romifz You can use the branch in #154 until we decide on the behavior.

Thanks!

duncandewhurst commented 3 years ago

Does anyone have an objection to changing the behavior so that the extension column indicates the extension that causes the JSON path to exist?

Sounds good to me.

yolile commented 3 years ago

Does anyone have an objection to changing the behavior so that the extension column indicates the extension that causes the JSON path to exist?

Sounds good to me too!

jpmckinney commented 3 years ago

OCDS Kit 0.2.11 is released with this update :)