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

fix: issues with members when dealing with non-numeric branches issue #906 #910

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

ioanaif
Copy link
Collaborator

@ioanaif ioanaif commented Jun 29, 2023

This issue was created when adding a way to deal with data having extra offsets.

For that, members were set to have a (None, None) item before headers were added (L234)

Then, in objects.py, the members var was cleared of the (None, None)s. (L600)

However, it was only tested in the case where a header is added after the Nones tuple, thus the while loop would stop at the first actual header and continue to clear the members var of Nones. In the file that caught this issue, no headers are appended and the members var in the end is equal only to (None, None) which causes this while loop to complain of index out of range.

*** I used range to fix the issue, because of the way the condition is imposed. If enumerate is used, pre-commit will complain of not using first_value_loc in the loop body.

@ioanaif ioanaif requested a review from jpivarski June 29, 2023 18:55
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

I used range to fix the issue, because of the way the condition is imposed. If enumerate is used, pre-commit will complain of not using first_value_loc in the loop body.

One code-tidying rule would have in one way ("Use enumerate if you use both the index and the value!") and another code-tidying rule would have it the other way ("Use _ if you don't reference the index in the loop!") I guess the rules are in conflict here because using break to intentionally get the first index value that satisfies some condition and use it beyond the loop itself is breaking the abstraction of what a loop is for.

(But it's fine to use it here because this is imperative programming; break to find the first index satisfying a condition is a common pattern.)

We talked about this on Zoom and I remember the context; this is the right fix.

I'll merge it now because it looks like a simple PR, and it looks done.

@jpivarski jpivarski merged commit 468b9c5 into main Jun 29, 2023
@jpivarski jpivarski deleted the ioanaif/fix-906-members-non-numerical branch June 29, 2023 21:55
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.

Exception on accessing .interpretation member for some non-numeric branches
2 participants