-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Batch: remove is_empty
#1108
Comments
@DarkTechPirate wanna have a look at this one? It's a fairly small thing and a good way to get started with contributing :) |
@MischaPanch yea sure |
Cool, thanks! |
Not sure what you mean. How is this related to |
in 5th line we are using |
Yes, you're right, hadn't seen it. I think a full-text search of |
@MischaPanch I'd be glad to assist with this one for review in PR. |
Sounds good, thanks! |
@DarkTechPirate do you have an ETA for when you could submit a PR? |
@MischaPanch so can we have a goggle meet or something as soon all my doubts clear i can finish it in 1 or 2 hours , let me know is it even possible ? |
@dantp-ai hey sure , we shall also have a meeting when you are free , lmk |
I won't have time until the end of next week, but @dantp-ai knows what this issue is about, so if the two of you have time to talk to each other, that would be the fastest option :). It's not a big change |
@DarkTechPirate I'd be happy to help. We can look at it tomorrow. |
@MischaPanch Okay sure , we will finish it off asap |
Thank you two, highly appreciate it! |
@dantp-ai how to contact you , discord or you people use something else !? |
@DarkTechPirate Once you have a solution (a work-in-progress is fine, though a complete solution is greatly appreciated), please open a PR and we can review the code together here on GitHub. The above description of the issue is instructive and should contain all the hints. If you have any doubts, feel free to write them down here and I will be able to help you. I assume you have already read the document on contributing to the codebase? |
@DarkTechPirate How can I help? |
@dantp-ai I have made some changes , but idk if it is correct or wrong , so let me send a pull request shortly and kindly can you go through it ? |
Yes, let's do like that. Looking forward to the PR. |
@DarkTechPirate I didn't see the PR. If you made some changes let's review them together in the PR even if you are unsure of the changes. |
okay |
It's not really a useful method and bloats the interface. The documentation already shows the intricacies of using it with
recurse=True
orrecurse=False
.It's more explicit to replace it with
batch.is_empty() -> len(batch.get_keys()) == 0
batch.is_empty(recurse=True) -> len(batch) == 0
Solving this means using the IDE's find usages, applying the above replacements, and removing
is_emtpy
fromBatch
andBatchProtocol
The text was updated successfully, but these errors were encountered: