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 7 forks source link

Replace deepcopy with more specific copys, to keep __reference__ #51

Closed Bjwebb closed 5 years ago

Bjwebb commented 6 years ago

Closes #42

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.5%) to 79.126% when pulling 2377dd5e8417db620901aa46b50794501874bd3a on 42-reference into c7aa01345a2d8b7c140faddf06412d34c1c707f6 on master.

jpmckinney commented 6 years ago

@duncandewhurst Can you run ocdskit with this branch, to see if the behavior is as you request in #42? If so, please feel free to merge.

duncandewhurst commented 6 years ago

@Bjwebb the behaviour now seems to be that the description from the reference is used in the flattened schema, rather than the description of the object being referred to.

I think that's an improvement on the existing behaviour, but it would be preferable to have a row for both the description from the reference and the description from the object being referred to, since sometimes important information is split across both, e.g. for budget, organizationReference and classification. Would it be a lot of work to add that functionality? (I described it in more detail in #42)

Bjwebb commented 6 years ago

@duncandewhurst Ah sorry, I'd missed that. I've added that functionality. I have mixed feelings about using reference for the type though, as its not the type that should be in the actual JSON file, so I worry it may cause confusion.

duncandewhurst commented 6 years ago

thanks @Bjwebb, good point on the use of reference, the other options would be:

  1. Use object as the type, so there would be two rows with the only difference being the description
  2. Concatenate the descriptions from the reference and the object being referred to into a single row

Neither of which seem ideal, unless there's anything else you can think of?

Bjwebb commented 6 years ago

I can't think of anything better. (1.) seems like the least bad option.

Bjwebb commented 6 years ago

I've implemented (1.) in my most recent commit.

jpmckinney commented 6 years ago

@duncandewhurst Please feel free to merge if the new output is what you expect/want.

jpmckinney commented 5 years ago

I assume the new output is fine.