-
Notifications
You must be signed in to change notification settings - Fork 5
Support reordering a child template in the Serverless ADR #377
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #377 +/- ##
==========================================
+ Coverage 81.19% 81.22% +0.02%
==========================================
Files 26 26
Lines 7515 7526 +11
Branches 1406 1408 +2
==========================================
+ Hits 6102 6113 +11
Misses 973 973
Partials 440 440 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@margalva I think it is the only follow-up PR, and a higher level wrapper seems not necessary because a Template object is needed whatsoever. Additionally, the current API is clean enough to use for both server and serverless ADR. Please let me know if this makes sense to you. |
raise TemplateDoesNotExist( | ||
f"Template with GUID '{target_guid}' is not found in the parent's children list." | ||
) | ||
self.children.remove(target_child_template) |
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 may not be reliable because there is no inherent equality check, which is used underneath .remove(). Let me push a quick fix.
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.
@zhang-yuanrui Please wait for this to be merged
#378
Then update yours with main.
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 like it uses guid to compare the object implicitly because the unit test passes. But indeed, the children field is marked non-comparable.
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.
Once that PR is merged, change the containment check above to:
if target_child_template not in self.children:
raise TemplateDoesNotExist(
f"Template with GUID '{target_guid}' is not found in the parent's children list."
)
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.
@zhang-yuanrui The unit test is passing because you are using the exact same object that you created all over the code, which is not something we can expect from a user. By default, if there is no equality check, it uses the internal instance id, not guid.
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.
If you did do a get() and reloaded it from the db after creation, the unit test will fail. It's a subtle bug
This PR is mainly to support reordering a child template in serverless ADR. It also moves 2 customized exceptions to a common file for both server and serverless.