-
Notifications
You must be signed in to change notification settings - Fork 58
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
[FIX] read annotation into NIMADs #914
Conversation
Reviewer's Guide by SourceryThis pull request fixes an issue where the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @jdkent - I've reviewed your changes - here's some feedback:
Overall Comments:
- It looks like you're initializing
self._annotations
in theStudyset
class constructor regardless of whether annotations are provided; consider removing the conditional logic. - Consider adding a test case that specifically covers the
metadata
attribute in theAnalysis
class.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@@ -292,6 +293,7 @@ def __init__(self, source): | |||
] | |||
self.images = [Image(i) for i in source["images"]] | |||
self.points = [Point(p) for p in source["points"]] | |||
self.metadata = source.get("metadata", {}) or {} |
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.
suggestion (bug_risk): Evaluate the use of 'or {}' with metadata initialization.
The expression source.get("metadata", {}) already provides a default empty dict when the key is missing, so the additional 'or {}' will override any false-y but valid metadata value. Consider whether this behavior is intentional or if an explicit test for None might be more appropriate.
Suggested implementation:
tmp = source.get("metadata")
self.metadata = {} if tmp is None else tmp
Review any other parts of the code that may depend on metadata being always a dictionary to ensure this change doesn't impact expected false-y values.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #914 +/- ##
=======================================
Coverage 88.23% 88.23%
=======================================
Files 48 48
Lines 6382 6383 +1
=======================================
+ Hits 5631 5632 +1
Misses 751 751 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #913
Changes proposed in this pull request:
Summary by Sourcery
Bug Fixes: