Skip to content

Make event_status and current_version optional on gene change events#326

Merged
sjm41 merged 4 commits into
mainfrom
feature/gene-change-event-optional-status-version
Jun 4, 2026
Merged

Make event_status and current_version optional on gene change events#326
sjm41 merged 4 commits into
mainfrom
feature/gene-change-event-optional-status-version

Conversation

@sjm41

@sjm41 sjm41 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Follow-up to Add GeneChangeEventSlotAnnotationDTO #323. Overrides required: false via slot_usage on both GeneChangeEventSlotAnnotation and GeneChangeEventSlotAnnotationDTO for event_status / event_status_name and current_version.
  • Slots remain required: true on the abstract ChangeEventSlotAnnotation / ChangeEventSlotAnnotationDTO parents — only the gene subclass relaxes them.

Test plan

  • CI (check-pull-request.yaml) regenerates artifacts and runs the full test suite

🤖 Generated with Claude Code

Override required: false via slot_usage on both GeneChangeEventSlotAnnotation
and GeneChangeEventSlotAnnotationDTO (event_status_name / current_version).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sjm41 sjm41 requested a review from chris-grove June 4, 2026 09:51
@claude

claude Bot commented Jun 4, 2026

Copy link
Copy Markdown

Summary: Follow-up to #323. Relaxes event_status/event_status_name and current_version from required: true to required: false on the gene change-event classes via slot_usage, leaving the abstract parents strict.

Assertions: None. The change is mechanically correct and consistent:

  • gene.yaml:192-195 overrides event_status + current_version on GeneChangeEventSlotAnnotation — both inherited from ChangeEventSlotAnnotation (core.yaml:403-404), defined required: true at core.yaml:1094/1109.
  • gene.yaml:219-222 mirrors this on the DTO with the correct suffixes: event_status_name (VocabularyTerm → _name) and current_version (plain integer → unchanged name), matching the parent DTO slot list (core.yaml:423-424). Entity/DTO divergence stays in sync. ✅

Questions (judgment calls for curators):

  • event_status and current_version are now optional on the entity class (GeneChangeEventSlotAnnotation), not just the DTO — i.e. the persistent store will accept gene change events without a status or version, not merely the ingest path. Is that intended, or should only the DTO (ingest) side be relaxed while the database keeps them required? The PR description frames this as relaxing both deliberately, so confirming the persistence intent is the only open point.

Otherwise this looks good — surgical and following the established slot_usage override pattern.

actions-user and others added 2 commits June 4, 2026 09:51
Per follow-up review, apply the required: false override on the abstract
ChangeEventSlotAnnotation and ChangeEventSlotAnnotationDTO classes via
slot_usage, rather than on the gene-specific child classes. Removes the
now-redundant overrides from GeneChangeEventSlotAnnotation and
GeneChangeEventSlotAnnotationDTO.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sjm41

sjm41 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Sorry, meant to make these changes on the parent classes, not just the gene-specific children. Now fixed as per 14f9ee0

@chris-grove chris-grove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also went ahead and removed the slot requirements at the level of the slot definitions, which is required in this case as we cannot override requirements for slots in slot_usage if they are defined as required in the slot definitions. I've made those changes and all tests pass so I'm approving this PR

@sjm41 sjm41 merged commit 789863d into main Jun 4, 2026
6 checks passed
@sjm41 sjm41 deleted the feature/gene-change-event-optional-status-version branch June 4, 2026 14:45
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.

3 participants