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

utils.py: Fixing assertion in drange to allow equal inputs #246

Merged
merged 5 commits into from
Oct 17, 2019

Conversation

abhmul
Copy link
Contributor

@abhmul abhmul commented May 17, 2019

We should allow drange to allow the same input value for v0 and v1. In this case, drange will return an iterator of one element: v0 // d.

I ran into an issue when using Camelot, where the values passed into v0 and v1 were the same. The original assertion in drange will fail when values for v0 and v1 are the same. However, I don't see why they can't be the same. In this case, drange will return a range iterator with just a single element equal to v0 // d. Making this change fixed the assertion without any incorrect change in the output of my code.

I am not very familiar with the codebases of pdfminer or Camelot, so I'd like an active contributor to take a look and see if there are any obvious issues. In the meanwhile, I'll continue to do some more investigation to see if this change could pose a problem somewhere, and I'll update this PR with anything I find.

We should allow drange to allow the same input value for v0 and v1. In this case, drange will return an iterator of one element: v0 // d.
@abhmul abhmul changed the title Fixing assertion in drange to allow equal inputs utils.py: Fixing assertion in drange to allow equal inputs May 17, 2019
@pietermarsman
Copy link
Member

pietermarsman commented Jul 9, 2019

This change would make drange different from the built-in range because the latter returns an empty list when the start and end are equal:

>>> list(range(0, 0))
[]

Changing drange the suggested way would be confusing since drange is obviously modelled after range().

Rather I would like drange to return an empty list if v0 <= v1 and d > 0.

@pietermarsman
Copy link
Member

Hi @abhmul, your PR seems to be related to #66. Do you have time to work on this?

Copy link
Member

@pietermarsman pietermarsman left a comment

Choose a reason for hiding this comment

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

I think this PR solves a problem in an unsatisfying manner.

drange should never throw an AssertionError when the inputs are wrong. It should return an empty list just like the builtin range.

@abhmul this is just a small change, can you do it?

@abhmul
Copy link
Contributor Author

abhmul commented Aug 19, 2019

Sure, I can do it this week, should be a quick fix.

@pietermarsman
Copy link
Member

I've checked and there is no except clause in pdfminer that catches a AssertionError. Thus the assertion can safely be replaced by returning an empty list like range does.

@pietermarsman pietermarsman changed the base branch from master to develop October 17, 2019 09:52
@pietermarsman pietermarsman self-requested a review October 17, 2019 09:55
@pietermarsman pietermarsman merged commit 7e40fde into pdfminer:develop Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants