Skip to content
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

Open
MischaPanch opened this issue Apr 12, 2024 · 24 comments · May be fixed by #1144
Open

Batch: remove is_empty #1108

MischaPanch opened this issue Apr 12, 2024 · 24 comments · May be fixed by #1144
Assignees
Labels
breaking changes Changes in public interfaces. Includes small changes or changes in keys good first issue Good for newcomers minor Requires small changes to be fixed refactoring No change to functionality

Comments

@MischaPanch
Copy link
Collaborator

It's not really a useful method and bloats the interface. The documentation already shows the intricacies of using it with recurse=True or recurse=False.

It's more explicit to replace it with

  1. batch.is_empty() -> len(batch.get_keys()) == 0
  2. 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 from Batch and BatchProtocol

@MischaPanch MischaPanch added good first issue Good for newcomers refactoring No change to functionality breaking changes Changes in public interfaces. Includes small changes or changes in keys minor Requires small changes to be fixed labels Apr 12, 2024
@MischaPanch
Copy link
Collaborator Author

@DarkTechPirate wanna have a look at this one? It's a fairly small thing and a good way to get started with contributing :)

@DarkTechPirate
Copy link

@MischaPanch yea sure

@MischaPanch
Copy link
Collaborator Author

Cool, thanks!

@DarkTechPirate
Copy link

image

can this also be changed into len(batch.get_keys()) == 0

@MischaPanch
Copy link
Collaborator Author

Not sure what you mean. How is this related to is_empty()?

@DarkTechPirate
Copy link

DarkTechPirate commented Apr 12, 2024

in 5th line we are using is_empty() to check it right!?
if we remove that fun it wont work , so we need to replace it with something right

@MischaPanch
Copy link
Collaborator Author

Yes, you're right, hadn't seen it. I think a full-text search of is_empty should reveal all usages, in case the find usages of your IDE misses some

@dantp-ai
Copy link
Contributor

@MischaPanch I'd be glad to assist with this one for review in PR.

@MischaPanch
Copy link
Collaborator Author

@MischaPanch I'd be glad to assist with this one for review in PR.

Sounds good, thanks!

@MischaPanch
Copy link
Collaborator Author

@DarkTechPirate do you have an ETA for when you could submit a PR?

@DarkTechPirate
Copy link

@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 ?

@DarkTechPirate
Copy link

@dantp-ai hey sure , we shall also have a meeting when you are free , lmk

@MischaPanch
Copy link
Collaborator Author

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

@dantp-ai
Copy link
Contributor

@DarkTechPirate I'd be happy to help. We can look at it tomorrow.

@DarkTechPirate
Copy link

@MischaPanch Okay sure , we will finish it off asap

@MischaPanch
Copy link
Collaborator Author

Thank you two, highly appreciate it!

@DarkTechPirate
Copy link

@dantp-ai how to contact you , discord or you people use something else !?
its my first time working with someone , so sorry for inconvenience!

@dantp-ai
Copy link
Contributor

dantp-ai commented Apr 18, 2024

@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?

@dantp-ai
Copy link
Contributor

@DarkTechPirate How can I help?

@DarkTechPirate
Copy link

@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 ?

@dantp-ai
Copy link
Contributor

Yes, let's do like that. Looking forward to the PR.

@dantp-ai
Copy link
Contributor

@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.
Pls add me as reviewer. Thank you!

@DarkTechPirate
Copy link

okay

@DarkTechPirate
Copy link

DarkTechPirate commented Apr 25, 2024

@dantp-ai check #1125
Seems like i kinda messed up , T_T

@dantp-ai dantp-ai mentioned this issue Apr 26, 2024
8 tasks
@dantp-ai dantp-ai linked a pull request May 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Changes in public interfaces. Includes small changes or changes in keys good first issue Good for newcomers minor Requires small changes to be fixed refactoring No change to functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants