Use [] as default affiliation value

[waxlamp]

This provides [] as a default value for Person.affiliation, which is typed as a list of Affiliation. The current default value of null causes problems when trying to process an empty affiliation value in the DANDI Archive meditor (see dandi/dandi-archive#2150).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.48%. Comparing base (c300776) to head (d078fbe).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
- Coverage   97.54%   97.48%   -0.06%     
==========================================
  Files          16       16              
  Lines        1751     1751              
==========================================
- Hits         1708     1707       -1     
- Misses         43       44       +1     

Flag	Coverage Δ
unittests	97.48% <100.00%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[waxlamp]

I don't know if this should be 0.6.10 or 0.7.0 (not sure what the policy is for changing a default value, even if just to fix a bug). Thoughts?

[candleindark]

I think it is most likely to be 0.6.10. @yarikoptic ?

[candleindark]

I am OK with the proposed change. It will be a bit inconsistent with many other similar definitions though, such as

dandi-schema/dandischema/models.py

Lines 1522 to 1526 in c300776

	studyTarget: Optional[List[str]] = Field(
	None,
	description="Objectives or specific questions of the study.",
	json_schema_extra={"nskey": "dandi"},
	)

.

[waxlamp]

I think, to the extent possible, those other definitions should be updated to also use []. This is because (1) we already ran into a UX bug due to the affiliation problem I'm addressing in this PR and (2) the jsonschema spec itself recommends that default values be valid according to the schema. I asked ChatGPT about reasons not to have a valid default value and it provided some examples (none of which seem to apply to us).

[candleindark]

I agree with you on that if we only consider the Pydantic models.

In fact, I was thinking changing affiliation: Optional[List[Affiliation]] to just affiliation: List[Affiliation], building on your proposed change, since we don't need both None and [] to indicate having no affiliation. However, doing so has at least two downsides.

1. We will have to migrate the existing metadata
2. There is no default value of an empty list in LinkML. We will be able to get a more precise translation of the models to LinkML if we keep the current definition.

[waxlamp]

I don't know how to solve the LinkML issue, but I agree with your point about changing the type to List[Affiliation]. It's true that it would require a data migration, but in my experience maintaining even slightly wrong types in systems like this tends to have compounding effects into the future (in our case, one of those effects would be the crash I'm trying to fix in the meditor). If we can figure out our approach to automatic migration of metadata, that may become less painful.

But, for now, you're thinking to not make that change in order to keep the blast radius from this PR smaller, is that right?

[candleindark]

Yes, I agree with you but let's keep it small for now. If it is not be cause of the LinkML translation, I would suggest changing to all similar type declaration to something like List[Affiliation] already.

[waxlamp]

@candleindark, I see that you approved this PR (thank you for the quick turnaround) but we still have an open question about the schema version, and there's a failing coverage test. Is there anything I should do to help advance this? Will you merge this when ready, or is that up to me?

[candleindark]

@waxlamp Everything looks fine for me. "All modified and coverable lines are covered by tests." I don't do merge on this repo. @yarikoptic and @satra are the ones with the final call on this repo.

[waxlamp]

In light of this discussion, I'll close this PR. We can revisit this later if needed.