-
Notifications
You must be signed in to change notification settings - Fork 667
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
feature/#1197 pass job as metadata for event #1277
feature/#1197 pass job as metadata for event #1277
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified 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.
- We are missing the unit tests covering the new code on both reload.py and submission.py modules.
- Even if the code looks smart, I don't like giving the notification responsibility to the entity itself. For me, it should be the repository/self_setter layer.
We can discuss and try to challenge your solution to see if we can propose a better design.
}[ | ||
manager | ||
] # type: ignore | ||
}[manager] # type: ignore |
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.
WHy do we need to add a "# type: ignore" here?
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.
apparently we don't need it!
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.
We should remove it, then.
44e9311
to
a47bac4
Compare
EventOperation.UPDATE, | ||
"submission_status", | ||
submission._submission_status, | ||
job_triggered_submission_status_changed=job.id, |
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.
This parameter name is exposed to the user through the event. It becomes an attribute of the event. We must find a better name. What about: "job_id"?
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.
a bit vague imho, how about trigger_job_id
?
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.
it is a submission status change. I can't see any possible confusion using job_id
instead of trigger_job_id
. Plus, it is a technical consideration for me to add a prefix like trigger
.
The event expresses a submission_status change for a submission entity due to a job update. I don't see any possible confusion.
What do you think? and @FlorianJacta?
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.
I agree
95b902f
to
73a2435
Compare
2add572
to
7d827c5
Compare
#1197