-
Notifications
You must be signed in to change notification settings - Fork 101
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
datachain: generalize data access functions into collect(), and collect_flatten #121
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
+ Coverage 85.71% 86.85% +1.13%
==========================================
Files 93 88 -5
Lines 9493 9364 -129
Branches 1897 1876 -21
==========================================
- Hits 8137 8133 -4
+ Misses 1024 899 -125
Partials 332 332
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
08b4fbc
to
d0637ff
Compare
Deploying datachain-documentation with Cloudflare Pages
|
d0637ff
to
f119a3a
Compare
f119a3a
to
06df3f1
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, now it's much clearer how to fetch data as those redundant methods were confusing.
e882ab2
to
7a091aa
Compare
Feel free to merge. I will handle conflicts (if any) and merge in the morning. |
def collect(self, col: str) -> Iterator[DataType]: ... # type: ignore[overload-overlap] | ||
|
||
@overload | ||
def collect(self, *cols: str) -> Iterator[tuple[DataType, ...]]: ... |
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 wish there was a way to force the output to be a tuple, working with a variable cols
is going to be awkward otherwise as the corner cases of len(cols) in [0,1]
need to be handled.
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. One idea that I have is we can make the first argument to be (arg: tuple[str] | str, *args: str)
.
But that complicates signature.
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!
Closes #67.