-
Notifications
You must be signed in to change notification settings - Fork 116
fix(setup): allow empty values for args #1230
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
Conversation
Reviewer's GuideModified row_to_objs in SignalSchema and the UDF parser to allow empty setup values by checking for key presence instead of truthiness, updated type annotations accordingly, and added a unit test for empty values. Sequence diagram for row_to_objs handling of empty setup valuessequenceDiagram
participant UDFParser
participant SignalSchema
UDFParser->>SignalSchema: row_to_objs(row)
SignalSchema->>SignalSchema: _init_setup_values()
alt setup_values contains name
SignalSchema-->>UDFParser: objs.append(setup_values.get(name))
else ModelStore.to_pydantic(fr_type) is not None
SignalSchema-->>UDFParser: objs.append(fr(**j))
end
Class diagram for updated SignalSchema and UDF parsingclassDiagram
class SignalSchema {
+row_to_objs(row: Sequence[Any]) list[Any]
- _init_setup_values()
values: dict
setup_values: dict
}
class UDFParser {
+_parse_row(row_dict: RowDict, catalog: Catalog, cache: bool, download_cb: Callback) list[Any]
params: SignalSchema
}
SignalSchema <.. UDFParser : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Deploying datachain-documentation with
|
Latest commit: |
c9ca06a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://6e1936f2.datachain-documentation.pages.dev |
Branch Preview URL: | https://fix-setup-empty-arg.datachain-documentation.pages.dev |
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 @shcheklein - I've reviewed your changes - here's some feedback:
- Double‐check that changing the setup check to
if name in setup_values
won’t accidentally override parsing for legitimately falsy values (like None or 0) that you might still want to unflatten instead of returning directly. - Since you widened the return types of
row_to_objs
and_parse_row
tolist[Any]
, consider whether you can still preserve a more specific type alias or narrow that type in downstream code to avoid losing type information. - In the test, using
dict
as the setup value actually refers to the constructor rather than an empty dict instance—consider passing{}
directly to make the intent clearer and avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Double‐check that changing the setup check to `if name in setup_values` won’t accidentally override parsing for legitimately falsy values (like None or 0) that you might still want to unflatten instead of returning directly.
- Since you widened the return types of `row_to_objs` and `_parse_row` to `list[Any]`, consider whether you can still preserve a more specific type alias or narrow that type in downstream code to avoid losing type information.
- In the test, using `dict` as the setup value actually refers to the constructor rather than an empty dict instance—consider passing `{}` directly to make the intent clearer and avoid confusion.
## Individual Comments
### Comment 1
<location> `tests/unit/lib/test_signal_schema.py:995` </location>
<code_context>
+ spec = {"name": str, "age": float, "init_val": int, "fr": MyType2, "empty": dict}
setup_value = 84635
- setup = {"init_val": lambda: setup_value}
+ setup = {"init_val": lambda: setup_value, "empty": dict}
schema = SignalSchema(spec, setup)
</code_context>
<issue_to_address>
Consider adding tests for other types of empty values (None, empty string, etc).
Please add test cases for 'empty': None and 'empty': "" to ensure all intended empty value types are covered.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
setup_value = 84635 | ||
setup = {"init_val": lambda: setup_value} | ||
setup = {"init_val": lambda: setup_value, "empty": dict} |
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 (testing): Consider adding tests for other types of empty values (None, empty string, etc).
Please add test cases for 'empty': None and 'empty': "" to ensure all intended empty value types are covered.
pos = 0 | ||
for name, fr_type in self.values.items(): | ||
if self.setup_values and (val := self.setup_values.get(name, None)): |
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.
Here: we skip it if value is not boolen(val)
. So, we skip if we have value set to {}
for example.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
==========================================
- Coverage 88.68% 88.66% -0.03%
==========================================
Files 152 152
Lines 13606 13605 -1
Branches 1893 1893
==========================================
- Hits 12067 12063 -4
- Misses 1093 1095 +2
- Partials 446 447 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
LG
Allow sending empty (
{}
,None
,""
, etc) values to setup. Before it was failing with an index error.Summary by Sourcery
Allow empty values to be used as setup arguments by adjusting the key check logic in row_to_objs and updating return types to list[Any]
Bug Fixes:
Enhancements:
Tests: