-
Notifications
You must be signed in to change notification settings - Fork 6
[ENH] annotation order #1220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH] annotation order #1220
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for key in ordered_keys: | ||
| descriptor = note_keys.get(key) or {} | ||
| note_type = descriptor.get("type") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normalize legacy note_keys without crashing
Existing annotations store note_keys as simple key -> "string" mappings, but _normalize_note_keys now assumes each entry is a dict with .get. When a PUT or pipeline application calls this helper on a legacy annotation, descriptor becomes a bare string and descriptor.get("type") raises an AttributeError, turning the request into a 500 rather than normalizing the data. The function needs to accept the old string format as well as the new descriptor objects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I'm just going to migrate the old format to the new format.
jdkent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a util to add numbers to the annotations. overall looks good.
| # add notes to annotation | ||
| annot.note_keys = { | ||
| k: _check_type(v) for k, v in annotation_row._asdict().items() | ||
| k: {"type": _check_type(v) or "string", "order": idx} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check_type has a fallback to string
closes #1202
in support of #1197