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 loadRes() to work with both Python 2 and 3. #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

waleedka
Copy link

This is a small fix for issue #49 to let loadRes() work on Python 2 & 3.

@hosang
Copy link

hosang commented May 18, 2017

If you want to avoid exceptions, you could use sys.version_info[0] >= 3 instead.

@waleedka
Copy link
Author

@hosang Good idea! Would this block merging the pull request? If so, I'll make the update. Otherwise, I think it's also common in Python to use exceptions for such cases. I've seen it used often in handing unavailable imports, converting strings to numbers, and so on.

@waleedka
Copy link
Author

waleedka commented Sep 4, 2017

Can someone merge this please! Your code is broken on Python 3 and this fixes it. It's been months since I did this pull request, and it's a tiny one. What's the hold up?

@ahundt
Copy link

ahundt commented Dec 21, 2017

@tylin @pdollar gently checking if the python 3 fix can be merged

ahundt added a commit to ahundt/cocoapi that referenced this pull request Dec 21, 2017
Fix loadRes() to work with both Python 2 and 3.
@scarroll32
Copy link

+1

@CMCDragonkai
Copy link

Wondering why this isn't merged yet?

@IssamLaradji
Copy link

Is there a reason why this pull request isn't merged yet?

@dheerajpai
Copy link

There is an alternaitive repo with the error rectified.

https://github.com/waleedka/coco

@gvoysey
Copy link

gvoysey commented Aug 10, 2018

This issue needs to be resolved. There are now at least three public forks to address py3 compatibility and use of numpy. This level of fracture is unnecessary in this case.

@DuaneNielsen
Copy link

DuaneNielsen commented Nov 23, 2018

Please merge this request! It's a simple button press.. thanks.

@vikiQiu
Copy link

vikiQiu commented Jul 8, 2019

Why this branch still not merged?? Is there anybody in charge?

@Akhil-Raj
Copy link

In case anybody is thinking why is the PR still not merged, I think there is no need for merging as the current code checks the 'PYTHON_VERSION' before checking for Unicode equality. So, there won't be any undefined name error.

@juskuz
Copy link

juskuz commented Jan 15, 2020

@Akhil-Raj so should it be working as is? no need to use waleedka's fork?

@pdollar can you merge it? or it should be working even without merging this PR?

@bertsky
Copy link

bertsky commented Mar 3, 2020

In case anybody is thinking why is the PR still not merged, I think there is no need for merging as the current code checks the 'PYTHON_VERSION' before checking for Unicode equality. So, there won't be any undefined name error.

That's not true (at least for pycocotools==2.0.0 from PyPI):

File "python3.6/site-packages/pycocotools/coco.py", line 308, in loadRes
    if type(resFile) == str or type(resFile) == unicode:
NameError: name 'unicode' is not defined

The same thing happens with installation from git (rev. 8c9bcc3).

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.