-
Notifications
You must be signed in to change notification settings - Fork 828
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
add verify for customized dependencies response #4982
add verify for customized dependencies response #4982
Conversation
/assign |
/assign |
It's just a question, how did you discover that optimization could be done here? |
I've been working on the #1741 issue recently. I read the code of the dependencies distributor module and found some confusing naming. So I submitted PR #4980. In the #4980, there is an error info output:
I think this error message should be discovered much earlier, so I submitted this PR |
d1b5dde
to
61a96f4
Compare
thanks for your quick response. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #4982 +/- ##
=======================================
Coverage 53.31% 53.32%
=======================================
Files 252 253 +1
Lines 20539 20554 +15
=======================================
+ Hits 10951 10960 +9
- Misses 8862 8869 +7
+ Partials 726 725 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
/lgtm
/cc @RainbowMango |
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.
Some nits, and a question:
Why not verify the dependencies returned from the default interpreters?
I think that the return body of external requests should be verified. The default interpreter does not involve external requests. It should be determined that the code is normal when it is written. |
@chaunceyjiang what's your opinion on this? |
61a96f4
to
b2d5be5
Compare
Good question! I believe third-party interpreters also need this. |
Yes, that's exactly the reason why I'm asking. |
b2d5be5
to
588635c
Compare
Validation for the InterpretDependency operation has been added for all interpreters. |
588635c
to
360c591
Compare
360c591
to
7fa3fea
Compare
…tion Signed-off-by: changzhen <changzhen5@huawei.com>
7fa3fea
to
8ec02ca
Compare
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
After the user customizes interpretation behavior, the Dependencies returned by the
InterpretDependency
interpreter are verified to prevent incorrect return values from penetrating into subsequent use logic.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: