faif/python-patterns

Updating the source to Python 3?

emmeowzing opened this issue ยท 16 comments

Just a quick comment--looking over the source, do you think it'd be a good idea to update to Python 3, e.g. using type annotations and Python 3 syntax?

+1 Python 2/3 compliant code
-1 Annotations (I don't think it's necessary at the moment)

faif commented

I second @pylang. Python 2/3 compatibility is more important than type annotations for this project

While we're on the topic of python 3, I am finding a difference in behaviour running the registry example.
https://github.com/faif/python-patterns/blob/master/behavioral/registry.py

With python 3.6 I do not see the ClassRegistree printed. Copied the code verbatim and ran it.

egens commented

In python 3 metaclass is added differently

class BaseRegisteredClass(object, metaclass=RegistryHolder):

instead of

class BaseRegisteredClass(object):
    __metaclass__ = RegistryHolder

https://stackoverflow.com/questions/5189232/how-to-auto-register-a-class-when-its-defined

More than a year since last comment here. Maybe it's time to remember about the topic? :)

Considering python2's EOL I would like to see python3-only code.
-1 for type annotations.

There are 11 months left until Python2 EOL ๐Ÿ˜‰ keeping the Python2/3 compatiblility is probably helpful for some people still working with Python2 code. But a Python3 branch would be nice.

-1 for type annotations.

faif commented

Agree with @fkromer Let's revisit this after the Python 2 EOL

There are a couple of examples with __metaclass__ syntax (registry, chain of responsibility, some more..).

  1. Syntax for Python 2 and 3 is different
  2. Being run with different python versions scripts produce different output .
  3. There is no way to write them in compatible way.
    I don't like workaround with six lib because
    • any dependency is unwanted
    • examples will lose both Python2 and Python3 look:
from six import with_metaclass

class MyClass(with_metaclass(Meta)):
    pass

Proposition:

  1. add note to README. smth like
DISCLAIMER
Although we try to keep 2/3 compatibility most outputs are given for Python3 by default. 
In some situations (e.g. when metaclasses are being used) there is no compatible syntax. This is why such scripts might fail with SyntaxError or give unexpected output for Python2.
Such scripts will have compatibility DISCLAIMER on the top.
  1. Update scripts to use Python3 only metaclass syntax and add DISCLAIMER on the top

This is different from "Update everything to Python3 syntax, use f-strings, etc etc ".
It is rather "Python 3 first, compatible with Python2 syntax as much as possible". In case of metaclasses it is just not possible (py2 syntax doesn't work in py3, wo errors. py3 syntax raises SyntaxError in py2)

faif commented

@gyermolenko How about using six until the Python 2 EOL? After that, we can get rid of it and keep only the Python 3 functionality

My issue with six is that code becomes neither py2 nor py3.
So whatever user is looking for, if he doesn't use six himself already, - he will need to modify script.

Examples below:

class Meta(type):
    pass

# Python 2 only:
class MyClass(Meta):
    __metaclass__ = Meta
    pass

# Python 3 only:
class MyClass(metaclass=Meta):
    pass

# with six
from six import with_metaclass

class MyClass(with_metaclass(Meta)):
    pass

ps. I am pro Python3-first, not Python3-only approach.
And so far only examples with metaclasses are controversial (that is 3-5 scripts).

faif commented

The maybe it's better to split those files to Python 2/3 versions until the Python 2 EOL

hey @faif
Not much time left till EOL.
I would like to clean some py2 specific things up if you don't mind.

So maybe it makes sense to create git tag or release and proceed with py3-only version in master?
Other options (which I don't like) would be:

  • proceed with maintaining 2/3 compatible code
  • maintain separate branches for 2 and 3

And with git tag later someone can switch from github ui or do git checkout past versions easily.

Please share your thoughts so I prepare changes accordingly.

faif commented

I like the tag solution. That way anyone still interested in the Python 2 solutions can access them. Letโ€™s cleanup master to use Python 3 only and have a reference to the tag.

I think only you can push new tags.

faif commented

@gyermolenko I pushed a legacy tag that contains the current repo contents. Feel free to start cleaning up ๐Ÿ‘

the issue can be closed