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

Set python3 as interpreter for doc tools #5348

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

juliantaylor
Copy link
Contributor

The scripts require python3 so the interpreter should use that.
python does not point to python3 on most distributions.

The scripts require python3 so the interpreter should use that.
python does not point to python3 on most distributions.
@josef-pkt
Copy link
Member

@bashtage @thequackdaddy any comments?
I have no idea about this, and it is not relevant on windows where these are ignored, i.e. I don't care either way..

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 84.207% when pulling 1da0694 on juliantaylor:fix-interpreters into 34ae057 on statsmodels:master.

@bashtage
Copy link
Member

I would probably reject this since python3 isn't necessarily available on all platforms. Essentially these should be run in a vurtual env with python 3.6+ as the default interpreter, in which case python is fine.

@juliantaylor
Copy link
Contributor Author

juliantaylor commented Oct 27, 2018

python3 should be available on all platforms where this execution method is available, see the recommendations of https://www.python.org/dev/peps/pep-0394/

By using python for python3 scripts you are breaking all unix like distributions except arch (where python3 also exists).

virtualenv should also have python3 available if not that is a bug as it is violating the pep.

@josef-pkt
Copy link
Member

related thread https://groups.google.com/forum/#!topic/comp.lang.python/YGwlPPxeVr8
I didn't really try to understand this because it is irrelevant for me, and I don't know much about the various Linux distributions.

@josef-pkt
Copy link
Member

to clarify the doc build requirements.

Does the doc build now require python 3 or does it still work with python 2.7 also?

If the former, then the change looks useful to force python 3.
If it works with either version, then a plain python would be appropriate, and leave it to the system to select the appropriate version.

@juliantaylor
Copy link
Contributor Author

it does not work in python2, the files use python3 features, e.g. futures or the encoding argument of open

@bashtage
Copy link
Member

The expectation of this file, and the doc generation in general, is that it is run in an environment where python 3 is the default python.

@juliantaylor
Copy link
Contributor Author

That assumption does not fit with pythons own pep. I do not see why statsmodels is different from all other python software in this regard that it must assume this non-standard behaviour.

@josef-pkt josef-pkt merged commit 981e02f into statsmodels:master Nov 28, 2018
@josef-pkt
Copy link
Member

merged,
IMO it is better to be explicit about the python version when we don't follow our standard py2/3 compatibility.

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.

4 participants