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

no basestring in py3.5 #47

Closed
sbailey opened this issue Sep 8, 2016 · 4 comments · Fixed by #48
Closed

no basestring in py3.5 #47

sbailey opened this issue Sep 8, 2016 · 4 comments · Fixed by #48

Comments

@sbailey
Copy link
Contributor

sbailey commented Sep 8, 2016

I'm confused about the following:

  • specsim uses basestring several times, e.g. simulator.py line 46
  • basestring doesn't exist in py3.5, so this code shouldn't work
  • But py3.5 tests pass and coverage claims that it touched that line

desisim usage of specsim breaks under py3.5 because it does touch that line and ends with:

======================================================================
ERROR: test_quickgen (desisim.test.test_quickgen.TestQuickgen)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/sbailey/desi/git/desisim/build/lib/desisim/test/test_quickgen.py", line 191, in test_quickgen
    desisim.scripts.quickgen.main(opts)
  File "/Users/sbailey/desi/git/desisim/build/lib/desisim/scripts/quickgen.py", line 166, in main
    qsim = specsim.simulator.Simulator(args.config)
  File "/Users/sbailey/desi/git/specsim/specsim/simulator.py", line 46, in __init__
    if isinstance(config, basestring):
NameError: name 'basestring' is not defined

It's easy enough to fix the basestring usage in specsim, but we should take a moment to figure out why tests + coverage are tricking us on this one, thus I'm opening this issue instead of proceeding directly to a PR.

@weaverba137
Copy link
Member

That is very strange. My initial guess was that basestring was being replaced with six.string_types with some astropy-affiliated magic, but I can't find any evidence for that.

@dkirkby
Copy link
Member

dkirkby commented Sep 8, 2016

I included use_2to3=True in setup.py, which I inherited from the astropy package template. This is supposed to run the 2to3 conversion when the package is installed in a python3 environment, including the basestring -> str substitution:

http://setuptools.readthedocs.io/en/latest/python3.html

I guess you didn't re-install when you switched from py2 to py3?

@weaverba137
Copy link
Member

It might not even be a case of installing (in the python setup.py install sense); most DESI code will work even from a git checkout, provided environment variables are set correctly. I don't know if we want to require that for specsim, since it is technically a standalone package.

@sbailey
Copy link
Contributor Author

sbailey commented Sep 9, 2016

Ben's guess is correct: I was using a "raw" checkout by just adding the top level directory to my PYTHONPATH without any "python setup.py install" or "python setup.py develop". I'll open a PR in a moment that converts the code to be genuinely py3 compatible rather than using 2to3 on-the-fly during install.

dkirkby added a commit that referenced this issue Sep 12, 2016
Be compatible with py2 and py3 without 2to3.  Fixes #47.
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 a pull request may close this issue.

3 participants