airr-community / airr-standards

AIRR Community Data Standards
https://docs.airr-community.org
Creative Commons Attribution 4.0 International
35 stars 23 forks source link

Genotype and MHCGenotype inconsistent nullable designations #736

Closed bcorrie closed 7 months ago

bcorrie commented 7 months ago

All fields in the sub-objects that are referred to in the Repertoire object have an explicit nullable attribute set.

In the airr-schema.yaml file, this is inconsistent for the MHCGenotype and Genotype sub-objects with many fields missing the nullable attribute:

https://github.com/airr-community/airr-standards/blob/13fc7b1c4bdcafb7fa04f3122aec1a365f9168b7/specs/airr-schema.yaml#L1304

In the OpenAPI 3.0 spec they are consistent with all fields having an explicit nullable attribute.

https://github.com/airr-community/airr-standards/blob/13fc7b1c4bdcafb7fa04f3122aec1a365f9168b7/specs/airr-schema-openapi3.yaml#L1388

So this is simply a matter of making the non OpenAPI 3.0 schema consistent with the the OpenAPI 3.0 schema.

javh commented 7 months ago

IIRC, we removed x-airr.nullable from the OpenAPI 2 germline schema while we were cleaning it up, because the default is nullable: true (ie, we only specify it in cases were nullable: false). I think the various Repertoire sub-objects are the odd man out.

The OpenAPI 3.0 schema works differently, so we're specifying it explicitly there.

bcorrie commented 7 months ago

Personally I prefer it to be explicit so it is clear in the spec. Was there a reason why we thought not having it specified and using a default is better?

One way or the other, the spec should probably be consistent???

javh commented 7 months ago

Easier to maintain and more likely to behave as expected. There are a lot of fields in the schema, so it's easier to specify a default for all of them than it is to fill all the x-airr attributes for each. (Extend the logic for identifier, adc-query-support, etc.)

bcorrie commented 7 months ago

OK, so Genotype is consistent with this, so closing the issue...