lukalabs / cakechat

CakeChat: Emotional Generative Dialog System
Apache License 2.0
1.7k stars 935 forks source link

Modernize Python 2 code to get ready for Python 3 #1

Closed cclauss closed 6 years ago

cclauss commented 6 years ago

Make the minimal, safe changes required to convert the repo's code to be syntax compatible with both Python 2 and Python 3. There may be other changes required to complete a port to Python 3 but this PR is a minimal, safe first step.

Run: futurize --stage1 -w */.py

See Stage 1: "safe" fixes http://python-future.org/automatic_conversion.html#stage-1-safe-fixes

One change that I was not so sure about was the change of a ur'string' to be just a u'string'. In Python 3, you can have u'strings' or r'strings' but ur'strings' are a syntax error.

./cakechat/utils/text_processing/str_processor.py:9:66: E999 SyntaxError: invalid syntax
_tokenizer = nltk.tokenize.RegexpTokenizer(pattern=ur'\w+|[^\w\s]')
                                                                 ^
1     E999 SyntaxError: invalid syntax
nikitos9000 commented 6 years ago

Hi @cclauss, thanks for your interest!

In my opinion, the best way to provide both python 2 and 3 compatibility is to use the six library or consider rewriting the code to be version-agnostic. I don't think that automatic replacement is a good option for now.

We'll create a feature-request list and try to fulfill the most popular requests.

cclauss commented 6 years ago

I too prefer the six approach but I often find that people are resistant to adding an additional dependency. I have modified the PR to use six instead of try/except blocks.

nikitos9000 commented 6 years ago

Great, six approach is much better, thanks :) We don't have enough tests yet, so I think we need to take a time to fully test your PR because it's a global modification.

I hope we'll return to this PR as everything will be properly tested.

khalman-m commented 6 years ago

Hi, @cclauss Thank you for your PR.

Unfortunately, the current version of the code is not compatible with Python 3. There are still some imports that should be done from six package. For example: https://github.com/cclauss/cakechat/blob/modernize-python2-code/cakechat/dialog_model/model_utils.py#L3

You can test that everything is working by running python bin/cakechat_server.py under Python 3. Can you please fix this and update the PR?

Thank you

cclauss commented 6 years ago

Closed in favor of #10