twoolie / NBT

Python Parser/Writer for the NBT file format, and it's container the RegionFile.
MIT License
362 stars 74 forks source link

Some Pep-8 and use xrange for python 2 #96

Closed suresttexas00 closed 6 years ago

suresttexas00 commented 6 years ago

Hello,

Since I was in the process of copying this into my own application, I made some changes you may find beneficial. It's mostly indentation and pep-8 things, but also introduces xrange usage which is implemented by using xrange but then aliasing range as xrange for python 3 . There are also a number of unused imports.

suresttexas00 commented 6 years ago

The failed tests for Python 2.6 and 3.2 do not appear related to the anything in the code changes I made.

macfreek commented 6 years ago

Thanks for your patch.

I agree that the code should be more PEP-8 compatible. for my own projects, I nowadays use flake8 and mypy to check the code. (The later is only useful for type hints, which NBT can't use in order to retain Python 2-compatible).

The only change I would make it to make the Python 3 syntax leading as much as possible. The reason is that I expect that Python 2-support will start to end in the next few years: Python itself said Python 2 will at least be supported till 2020 (10 years after it was released), and Red Hat said the default on RHEL 8 will be Python 3.

So my practical suggestion is to define range = xrange on Python 2, rather than defining range = xrange on Python 3.

suresttexas00 commented 6 years ago

Changing range = xrange for python 2, I would think is a bad practice, since range is already defined by python 2. The choice to define xrange = range for python 3 is based on: 1) xrange is not a python 3 built-in, so no name conflict/shadowing. 2) The code already defines our sting (unicode, basestring) methods based on Python 3.

macfreek commented 6 years ago

Your code was committed in version 1.5.0, albeit with range = xrange. I personally don't regard it as any different from a decorator, which also replaces one object with another object with compatible API.

If you think this causes genuine confusion, I'm happy to remove these lines. I personally don't think there is any benefit in keeping them: range is only used once in the code with a rather small length, so I don't think there is any performance difference between range and xrange in this code.

suresttexas00 commented 6 years ago

In retrospect, it is probably better to only implement the difference in Python 2 anyway. BTW thanks for all the work getting the new version implemented for the new snapshots.

macfreek commented 6 years ago

Surest Texas, that was a rather kind reply. Thank you! In retrospect, I took it a bit too serious. I remember we had a similar discussion on Look-before-you-leap versus Easier-to-ask-for-forgiveness-than-permission. The actual changes are minor, and mostly a matter of taste (I have a hard time coming up with examples were there is any difference in functionality). This is actually the first time I felt we rather should have this discussion over a drink instead of a Github ticket :).

I very much appreciate thoughtful replies to my ramblings. I like it that you force me to explain my considerations.

In this case, I should have admitted that your code is a fraction prettier. I suspect that my considerations are partially influenced by the wish to drop support for Python 2 (I did so recently in some private and work projects, and I didn't get much joy in still supporting it here).

suresttexas00 commented 6 years ago

I agree with your distaste for continuing Python 2 support. At this point, it is making the coding process more tedious.

I have also benefited from your explanations and have learned a great deal by following your work here.

We all have pre-conceptions that color our outlook. Sometimes, things like code prettiness, despite what the Zen of Python says: "There should be one-- and preferably only one --obvious way to do it. Although that way may not be obvious at first unless you're Dutch.", maybe sometimes, it is just a matter of personal opinion.

macfreek commented 6 years ago

"There should be one-- and preferably only one --obvious way to do it."

If only that was true! In reality, it is one of the biggest fallacies I've heard.

There are now four different ways for string formatting:

"Hello %s" % name
"Hello {0}".format(name)
f"Hello {name}"
string.Template('Hello $name!').substitute(name=name)

The third is new in Python 3.6. It seems nice and powerful, but I expect that in 5 years time we see lot of attacks where Django or Flask or whatever web apps allow users to customize that template, and hackers that discover that you can not only change the template to f"Hello {name.title()}" to change the case, but also to f"Hello {name.user_method.__globals__.config['password']". (where user_method is just any method in any class. __init__ is a good guess for an exciting one)

Of course, this should not be any surprise. API change over time, thus creating more than one way to do things. Even the taste of our benevolent dictator for life changes his preferences. Think about the functional elements in the languages: map, filter, reduce. They have been at time part of Python, not part of Python, build-in function, and part of the standard library. In fact, I would argue if there only was one way to do things, we wouldn't have this many programming languages to choose from. :)