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

Bugfix: check for exactly one line in call_git_oneline #29

Closed

Conversation

christian-monch
Copy link
Contributor

This PR prevents a possible IndexError in call_git_oneline if git returns less than one line.

This commit prevents a possible `IndexError` in
`call_git_oneline` if `git` returns less than
one line.
@christian-monch christian-monch requested a review from mih as a code owner October 25, 2024 09:27
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (6393bec) to head (7573cf7).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #29   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           54        54           
  Lines         2333      2333           
  Branches       162       162           
=========================================
  Hits          2333      2333           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first helper of this kind was introduced 5 years ago with datalad/datalad@8b30f4d
and came with this exact behavior.

Can you elaborate on why you'd want to have an AssertionError rather than an IndexError?

@christian-monch
Copy link
Contributor Author

christian-monch commented Oct 26, 2024

[...]
Can you elaborate on why you'd want to have an AssertionError rather than an IndexError?

I don't care too much about the error itself, but here are two thoughts about the current situation:

  1. The assertion says 'Expected Git {args} to return a single line, but got {lines}'. That is not what is checked by the if-statement. I think we should either change the comparison to len(lines) == 1 or the message to something like 'Expected Git {args} to return zero or one line, but got {lines}'. Since we only know after the if-clause that lines has zero or one element, it seems weird to "insist" (by using lines[0]) on returning one line. As a side note, the new message would clash with the name of the function, which seems not very nice.
  2. It is easier to find an error if an AssertionError with a correct message is raised, than to check why an IndexError occurred, because the AssertionError holds more context information.

@mih
Copy link
Member

mih commented Oct 28, 2024

Alright. Thanks. I added a test that verifies the behavior, and opened #33

This PR can be closed now.

@mih mih closed this Oct 28, 2024
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