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 - is_empty() #1125

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

DarkTechPirate
Copy link

  • I have added the correct label(s) to this Pull Request or linked the relevant issue(s)
  • I have provided a description of the changes in this Pull Request
  • I have added documentation for my changes
  • If applicable, I have added tests to cover my changes.
  • I have reformatted the code using poe format
  • I have checked style and types with poe lint and poe type-check
  • (Optional) I ran tests locally with poe test
    (or a subset of them with poe test-reduced) ,and they pass
  • (Optional) I have tested that documentation builds correctly with poe doc-build

@DarkTechPirate
Copy link
Author

@dantp-ai

@dantp-ai
Copy link
Contributor

dantp-ai commented Apr 26, 2024

The linting failed. Plz make sure to run poe format and poe lint before committing.

Also additionally, make sure to run poe type-check to resolve any typing issues.

@dantp-ai
Copy link
Contributor

dantp-ai commented Apr 26, 2024

@DarkTechPirate Plz make sure to perform the replacements (as specified in the description of the issue #1108) that you did in the batch.py also in the test files and in all other places where the usage of is_empty on a Batch occurs. Here, it would help again to search for the usage using the IDE.

For example, if you look into the output of one of the failed checks on Github here (by scrolling down to the "short test summary info"), you can see all the test files where the error AttributeError: 'dict' object has no attribute 'is_empty' happened. This means that is_empty is still being used on a Batch object, which fails because the method does not exist anymore.

Afterward you can run the tests locally before pushing. Try to run at least the reduced test suite with poe test-reduced and check for any issues.

@dantp-ai
Copy link
Contributor

dantp-ai commented Apr 26, 2024

@DarkTechPirate Make sure to catch all .is_empty usages on a Batch. For example here I used PyCharm and found these occurrences (note that it comes up also in comments and tutorial files).

image

@DarkTechPirate
Copy link
Author

@dantp-ai okay i got it , will update it and let you know .

@dantp-ai
Copy link
Contributor

dantp-ai commented May 2, 2024

@DarkTechPirate I wanted to touch base regarding the status of the PR. If you have any questions or need further clarification on the review comments, please feel free to reach out.

I'm here to help, and happy to discuss any aspects of the changes in more detail or assist you in implementing them if needed.

@MischaPanch
Copy link
Collaborator

@dantp-ai I guess we should take it over

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants