scragg0x / realms-wiki

Git based wiki inspired by Gollum
http://realms.io
GNU General Public License v2.0
833 stars 91 forks source link

systematic use of `from __future__ import absolute_import` #174

Closed stephane-martin closed 7 years ago

stephane-martin commented 7 years ago

It eliminates import NAME confusion with very common names (eg. import ldap is very ambiguous: can be a ldap module from realms-wiki, a ldap module from flask-ldap-login, or python-ldap module.

Also cleant up a bit the imports towards PEP8: builtin modules first, then 3rd party python packages, then local packages

thanks! stephane

gazpachoking commented 7 years ago

I think the explicit from realms.whatever imports are much nicer than the from ... imports. I'm not against the absolute_import though. I noticed you also started importing from six, I don't really thing that should be done without adding it explicitly to the requirements, and I don't really see a reason to do that unless we are trying to support python 3 as well. Did you try on python 3? Was that the motivation?

stephane-martin commented 7 years ago
nakato commented 7 years ago

I quite like the removal of .foo imports in flavor of bar.foo imports, as well as the ordering fixes.

With six being imported, regardless of other projects pulling it in, realms must list it as a requirement as it has become a primary user of six with this patch.

While I would love to see Py3 support personally, not all modules that realms pulls in support Py3, one being the git library, and another being the LDAP lib, as well as others. Ripping gittle/dulwitch out for gitpython isn't too hard though.

My branch has diverged a bit too much to get a diff from, but feel free to see how I used gitpython here for your future patches.

stephane-martin commented 7 years ago

in the current PR:

anything else you would like to see ?

gazpachoking commented 7 years ago

There are still a few more relative imports (including a couple I didn't comment on,) and this needs merging with the master changes now, but I like all this stuff. @scragg0x ?

stephane-martin commented 7 years ago

Hello,

to be clear, the problem is not relative imports per se, but implicit relative imports. The important part in the PR is the from __future__ import absolute_import.

I fixed the last stuff that you pointed. In other files I kept some simple relative imports when they were more readable.

gazpachoking commented 7 years ago

I fixed the last stuff that you pointed. In other files I kept some simple relative imports when they were more readable.

Ahh, I thought part of the plan was to standardize everything to absolute imports. I see some of those auth imports are pretty long without the relative stuff though, I'm fine with either way.

stephane-martin commented 7 years ago

rebased

gazpachoking commented 7 years ago

I still prefer absolute imports everywhere, but I think this is an improvement either way. Once the merge conflict is resolved I'm good to go.

stephane-martin commented 7 years ago

I fixed the glitch, thanks :)

Next PR will propose to remove/fix some python 3 incompatibilities.

stephane-martin commented 7 years ago

@nakato thanks for comment and advices :)

i think gittle has been removed, and dulwich says it's compatible python 3.5. so is it really necessary to replace dulwich with gitpython ?

for LDAP, work is in progress in a PR.