-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: [FC-0074] add support for optional trigger information #143
Conversation
Thanks for the pull request, @mariajgrimaldi! This repository is currently maintained by @bmtcril. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
3802408
to
626d2a4
Compare
I'm also going to defer this review to @bmtcril |
I would love to have this functionality, but I'm concerned that this implementation is too limiting. For instance if an event is triggered from multiple places, how would that be handled? I think it would also be challenging to keep the path references up to date when the annotations may be defined far away from the code. |
@bmtcril: Thanks for the review! This is how it looks for events triggered in multiple locations. It's simply a list of paths, each with its own GH search link. Can you explain further what you mean by I think it would also be challenging to keep the path references up to date when the annotations may be defined far away from the code? Also, what do you think would make this less limiting? |
Ah that's great, I had somehow forgotten that you could have repeating annotations.
My concern is that the event annotations live in openedx-events, but the places where events are triggered are elsewhere. So it would be hard to remember that when you refactor and move where an event is triggered, or add a new place, or remove it that you also need to go and update openedx-events to keep the docs up to date. I don't have a solution for that right now, but I think it's an important consideration. |
@bmtcril: totally. What we can do is change the GH search to something like https://github.com/search?q=repo%3Aopenedx%2Fedx-platform+CONTENT_OBJECT_ASSOCIATIONS_CHANGED.send_event&type=code, with the repo + event name.send_event, so at least we can inspect where the event is sent from without hardcoding the path. What do you think? I think this approach would be more maintainable over time since it's less likely that the repository where the event would change frequently. Here's what the change would look like: f82bf19 |
"code_annotations") / os.path.join("contrib", "config", "setting_annotations.yaml") | ||
|
||
OPENEDX_EVENTS_ANNOTATIONS_CONFIG_PATH = importlib_resources.files( | ||
OPENEDX_EVENTS_ANNOTATIONS_CONFIG_PATH = importlib.resources.files( |
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.
Reference: #135 (comment)
That looks perfect, thank you! |
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.
Looks good once tests pass
@bmtcril, done, thank you! You can review the results here: https://docsopenedxorg--443.org.readthedocs.build/projects/openedx-events/en/443/reference/events.html |
Thanks! This is released in v2.2.0 |
Description
Add triggering fields for opened-events annotations so developers can include relevant information about where the event is being triggered by rendering a link that redirects to a GH search with the source file and the event name.
Testing Instructions
event_trigger_repository
andevent_trigger_path
make docs
You can also review the rendered docs here: https://docsopenedxorg--443.org.readthedocs.build/projects/openedx-events/en/443/
Merge Checklist:
Post Merge:
finished.