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

Don't check for 'linux2' value in sys.platform #549

Closed
fperez opened this Issue Jul 1, 2011 · 7 comments

Comments

Projects
None yet
3 participants
@fperez
Member

fperez commented Jul 1, 2011

The linux kernel is transitioning to version 3.0, and this means that the sys.platform string will become on such machines 'linux3' instead. Right now, we have explicit checks for linux2:

(master)amirbar[IPython]> grin linux2
./quarantine/InterpreterExec.py:
  139 :         sys.platform is: linux2
./testing/decorators.py:
  300 : skip_linux = skipif(sys.platform == 'linux2',
  308 : skip_if_not_linux = skipif(sys.platform != 'linux2',
./testing/tests/test_decorators.py:
  179 :     nt.assert_not_equals(sys.platform,'linux2',"This test can't run under linux")
./utils/sysinfo.py:
  129 :      'sys_platform': 'linux2',

All these should become variants of 'foo[:5] == 'linux'.

I'll tag this as 0.11 in case we can make it, as it's an easy fix and it would be nice to run out of the box for people who upgrade kernels early. But it's not super critical.

@rkern

This comment has been minimized.

Show comment
Hide comment
@rkern

rkern Jul 1, 2011

Contributor

A more idiomatic version:

foo.startswith('linux')
Contributor

rkern commented Jul 1, 2011

A more idiomatic version:

foo.startswith('linux')
@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jul 1, 2011

Member

I suggested the uglier one b/c it's a little faster, I had checked both:

In [2]: timeit sys.platform.startswith('linux')
1000000 loops, best of 3: 294 ns per loop

In [3]: timeit sys.platform[:5] == 'linux'
1000000 loops, best of 3: 210 ns per loop

I know that 85ns may be a silly thing to worry about, but I tend to be a bit anal about avoiding slowdowns everywhere. Do you see another reason for sticking with the startswith form?

Member

fperez commented Jul 1, 2011

I suggested the uglier one b/c it's a little faster, I had checked both:

In [2]: timeit sys.platform.startswith('linux')
1000000 loops, best of 3: 294 ns per loop

In [3]: timeit sys.platform[:5] == 'linux'
1000000 loops, best of 3: 210 ns per loop

I know that 85ns may be a silly thing to worry about, but I tend to be a bit anal about avoiding slowdowns everywhere. Do you see another reason for sticking with the startswith form?

@rkern

This comment has been minimized.

Show comment
Hide comment
@rkern

rkern Jul 1, 2011

Contributor

Just general clean-code, DRY principles, nothing major.

Contributor

rkern commented Jul 1, 2011

Just general clean-code, DRY principles, nothing major.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jul 1, 2011

Member

Sure. I do tend to use startswith in general, to be honest. 80ns isn't really worth worrying about, and the functional version does read more cleanly, so let's go with that. I need to get back on my OCD meds... :)

Member

fperez commented Jul 1, 2011

Sure. I do tend to use startswith in general, to be honest. 80ns isn't really worth worrying about, and the functional version does read more cleanly, so let's go with that. I need to get back on my OCD meds... :)

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jul 1, 2011

Member

I've got this locally using startswith, should I just push to master?

Member

minrk commented Jul 1, 2011

I've got this locally using startswith, should I just push to master?

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jul 1, 2011

Member

On Thu, Jun 30, 2011 at 10:04 PM, minrk
reply@reply.github.com
wrote:

I've got this locally using startswith, should I just push to master?

Go for it, thanks!

Member

fperez commented Jul 1, 2011

On Thu, Jun 30, 2011 at 10:04 PM, minrk
reply@reply.github.com
wrote:

I've got this locally using startswith, should I just push to master?

Go for it, thanks!

@minrk minrk closed this in e1d1d8e Jul 1, 2011

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jul 1, 2011

Member

pushed and closed

Member

minrk commented Jul 1, 2011

pushed and closed

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014

check linux with startswith('linux') instead of =='linux2'
This allows for sys.platform=='linux3'.

closes ipythongh-549
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment