Skip to content
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

Process mention on annotation creation #9322

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

mtomilov
Copy link
Contributor

@mtomilov mtomilov commented Feb 3, 2025

Refs #9304

See testing instructions in the PoC #9279

Most everything has been addressed in the PoC

@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from cc37bf2 to 6dfab59 Compare February 4, 2025 11:47
@mtomilov mtomilov marked this pull request as ready for review February 4, 2025 14:54
mtomilov added a commit that referenced this pull request Feb 5, 2025
@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from be88a11 to 7980259 Compare February 5, 2025 11:17
mtomilov added a commit that referenced this pull request Feb 5, 2025
@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from 7980259 to 008f84f Compare February 5, 2025 11:20
@mtomilov mtomilov requested review from marcospri and acelaya February 5, 2025 11:21
mtomilov added a commit that referenced this pull request Feb 5, 2025
@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from 008f84f to 42922ed Compare February 5, 2025 11:25
@mtomilov
Copy link
Contributor Author

mtomilov commented Feb 5, 2025

Not entirely sure if I need to add view / functional tests

Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

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

Looks good to me in general. My main feedback is that we should probably run the mentions-specific logic only if the at_mentions feature is enabled.

h/services/annotation_json.py Outdated Show resolved Hide resolved
h/services/annotation_write.py Outdated Show resolved Hide resolved
h/services/mention.py Outdated Show resolved Hide resolved
mtomilov added a commit that referenced this pull request Feb 5, 2025
@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from 42922ed to 510fc62 Compare February 5, 2025 13:55
mtomilov added a commit that referenced this pull request Feb 5, 2025
@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from 510fc62 to 5ce9754 Compare February 5, 2025 14:43
mtomilov added a commit that referenced this pull request Feb 5, 2025
@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from 5ce9754 to 85f0de1 Compare February 5, 2025 14:45
Copy link
Member

@marcospri marcospri left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I might have split this into two PRs, one for processing the mentions and another one to expose them the API (addressing the latest changes there).

But this PR comes after the PoC one so we are already familiar with the changes anyway. The API field name change can be addressed in a follow up PR as you say 👍

h/services/mention.py Show resolved Hide resolved
h/services/annotation_write.py Outdated Show resolved Hide resolved
h/services/mention.py Show resolved Hide resolved
mtomilov added a commit that referenced this pull request Feb 6, 2025
@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from 85f0de1 to 13181bc Compare February 6, 2025 17:41
mtomilov added a commit that referenced this pull request Feb 6, 2025
@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from 13181bc to 06563e6 Compare February 6, 2025 17:52
@mtomilov mtomilov force-pushed the process-mention-on-annotation-creation branch from 06563e6 to dd7d8ee Compare February 7, 2025 11:44
@mtomilov mtomilov merged commit a3a53e9 into main Feb 7, 2025
11 checks passed
@mtomilov mtomilov deleted the process-mention-on-annotation-creation branch February 7, 2025 11:47
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