search

tonysimpson / nanomsg-python

nanomsg wrapper for python with multiple backends (CPython and ctypes) should support 2/3 and Pypy
MIT License
382 stars 85 forks source link

Add osx support to ctypes wrapper #46

Closed jbester closed 8 years ago

ryanbahneman commented 8 years ago

I was just about to do this. Do we know why the AppVeyor is failing? It seems like none of the other pull requests rand the AppVeyor check.

tonysimpson commented 8 years ago

Hey I'm happy to merge this, thanks for the patch!

Regarding Appveyor: I was playing around with it to see it if would be a practical way to build release binaries. I did not in any way ask github to tie the Appveyor project to the build or to report it's status here, I'm unimpressed that github has decided to do this, it will be what most people want but to do it without requiring e.g. a .appveyor file to show you opt in is annoying - I will disable the webhooks for Appveyor.

Little note about Python, it would have been better to have 

elif sys.platform == 'darwin':

Rather than

elif sys.platform in ('darwin'):

because you are actually writing 'darwin' in 'darwin' which is a bit silly - but I don't mind - the in keyword when the right hand side is a string does an fast substring match which will return True in this case but is unnecessary. What is really happening is sys.platform is the string 'darwin' and ('darwin') isn't a tuple it's just 'darwin' with brackets around it, you need the , to make a tuple e.g.

('I'm a tuple with this sting in it',)
('I'm not a tuple im just a string in brackets which are stripped out by the parser')

also:

(1 + 2,) == (3,)
(1 + 2)  == 3

The reason why I think this code is not idea is that the additional complexity may be misleading and cause someone to waste time assuming more is going on than is really.

Regardless thanks for the patch!!

I should post another issue regarding the future of nanomsg-python which might be interesting to both of you.