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

Annotated count fix index error #1707

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

Chr0nos
Copy link
Contributor

@Chr0nos Chr0nos commented Sep 11, 2024

Description

Motivation and Context

It fixes a 4 year old bug, when creating an annotation that has no matches, the result is an empty list but the actual code tries to reach for result[0] causing an IndexError

#469

How Has This Been Tested?

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@abondar
Copy link
Member

abondar commented Sep 26, 2024

Hi!

Please do not bump version in pyproject, I will do it myself of release

Can you please rebase on new develop, previous commit had faulty version of mssql there, so CI was failing

Also - would be great if you could add testcase, that demonstrates fixed behaviour

@Chr0nos Chr0nos force-pushed the annotated-count-fix-index-error branch 2 times, most recently from ceef62c to 8706fb1 Compare September 26, 2024 09:19
@Chr0nos
Copy link
Contributor Author

Chr0nos commented Sep 26, 2024

Hi!
just rebased now and removed the version bump, i'l try to add the testcase this weekend 👍

@Chr0nos Chr0nos force-pushed the annotated-count-fix-index-error branch from 8706fb1 to 827e278 Compare September 26, 2024 09:26
@coveralls
Copy link

coveralls commented Sep 26, 2024

Pull Request Test Coverage Report for Build 11232041104

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 38 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.005%) to 89.006%

Files with Coverage Reduction New Missed Lines %
tortoise/backends/mysql/executor.py 2 95.74%
tortoise/filters.py 10 91.3%
tortoise/contrib/pylint/init.py 26 0.0%
Totals Coverage Status
Change from base Build 11048931810: -0.005%
Covered Lines: 5975
Relevant Lines: 6598

💛 - Coveralls

@Chr0nos
Copy link
Contributor Author

Chr0nos commented Oct 10, 2024

Hi, i added the test that show what the fix is about, sorry for the response time

@abondar abondar merged commit 25d0624 into tortoise:develop Oct 10, 2024
7 checks passed
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.

3 participants