Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings
This repository was archived by the owner on Nov 22, 2024. It is now read-only.

Refactor imports#26

Merged
mahtin merged 1 commit into
cloudflare:mastercloudflare/python-cloudflare:masterfrom
yesbox:refactor_importsyesbox/python-cloudflare:refactor_importsCopy head branch name to clipboard
Dec 30, 2016
Merged

Refactor imports#26
mahtin merged 1 commit into
cloudflare:mastercloudflare/python-cloudflare:masterfrom
yesbox:refactor_importsyesbox/python-cloudflare:refactor_importsCopy head branch name to clipboard

Conversation

@yesbox

@yesbox yesbox commented Dec 30, 2016 •
edited
Loading

Copy link
Copy Markdown

This is a refactor of the imports in the CloudFlare library. I was prompted to make these changes when trying to use the cli4 utility with Python 3.5 and it wouldn't run. I have done a quick test with the changes on Python 2.6, 2.7. 3.3, 3.4 and 3.5.

Improved compatibility

  • Uses absolute imports for Python 3 compatibility.
  • ConfigParser has changed name and will now be imported correctly on Python 3. This error was hidden by the __init__.py try-except block.

Robustness and clear errors

  • Removed the try-except block from __init__.py. This was added in commit 110870b on the basis that "python3 does not need the import statement", which I cannot reproduce. If this import statement is removed then cli4 will not work with Python 3. Because this block allows real errors to pass silently that would have made it imminently more clear what's wrong it has been removed and the hidden errors fixed.
  • Moved __version__ to the top of __init__.py (before the class import). This must be done because the import of CloudFlare module from the cli4 utility or other scripts will in turn import utility.py which attempts to import __version__, but __version__ has not been set up at that point so the import will fail due to this layout. This error was hidden by the __init__.py try-except block.
  • Removes import of CloudFlare from setup.py. The import of the library before installation will fail on systems that do not already have all the dependencies that the setup is meant to install already, thereby failing the entire installation. Instead extract the version number with a regex, treating the __init__.py file just as text rather than code, that way there is still just one location for the version number. This is the root cause of Remove Broken Import #9.

Cleaning up

  • Removes some unused stdlib imports (urllib, os, sys).
  • Removes superfluous import of CloudFlare.exceptions. No use since the top module CloudFlare is always imported in these cases.
  • Removes "#!/usr/bin/env python" from non-script file converters.py.
  • Removes usage of sys.path.insert(0, os.path.abspath('..')). No need for this workaround when absolute imports are used.

Documentation and discoverability

  • Only import CloudFlare, not CloudFlare.exceptions (same as the code change).
  • Adds the CloudFlare class to the __all__ variable in __init__.py. This will produce useful output when issuing "import CloudFlare; help(CloudFlare)".

Not done (and don't intend to)
I have not changed the test or the examples.

@yesbox
Refactor imports
c013305 
Improved compatibility, robustness, clear errors and some cleaning up.
Also synced with the readme.
@mahtin mahtin merged commit c013305 into cloudflare:master Dec 30, 2016
@mahtin

mahtin commented Dec 30, 2016

Copy link
Copy Markdown

@yesbox - I've merged this and it looks good. I've added edits for removing import CloudFlare.exceptions from various examples/*.py files along with one final mention of it within ``README.md```.

However, this now introduces a bug within Python2 which stops cli4 testing during development.

mac:python-cloudflare martin$ cli4/__main__.py /ips
Traceback (most recent call last):
  File "cli4/__main__.py", line 7, in <module>
    from .cli4 import cli4
ValueError: Attempted relative import in non-package
mac:python-cloudflare martin$

mac:python-cloudflare martin$ cd cli4/
mac:cli4 martin$ ./__main__.py /ips
Traceback (most recent call last):
  File "./__main__.py", line 7, in <module>
    from .cli4 import cli4
ValueError: Attempted relative import in non-package
mac:cli4 martin$ cd ..
mac:python-cloudflare martin$

The latter would work and was my primary method of testing code. Do you have an answer to that issue?

Note that examples still work as they use the relative CloudFlare library due to sys.path.insert(0, os.path.abspath('..')) in their code.

@yesbox

yesbox commented Dec 30, 2016 •
edited
Loading

Copy link
Copy Markdown
Author

There are a couple of ways, sorted by effort (and payoff).

You can have Python run the package as a module:

$ python --version
Python 2.7.12
$
$ pwd
~/proj/cloudflare
$
$ python -m cli4
usage: cli4 [-V|--version] [-h|--help] [-v|--verbose] [-q|--quiet] [-j|--json] [-y|--yaml] [-r|--raw] [-d|--dump] [--get|--patch|--post|--put|--delete] [item=value ...] /command...

You can make an editable install, this will make the package act as in production but you can edit it live without having the reinstall to test your changes. The --user option installs the package in ~/.local/, but since combined with --editable you would still edit the files in the source, not in ~/.local/:

$ pwd
~
$
$ pip install --user --editable proj/cloudflare
Successfully installed cloudflare
$
$ python -c 'import CloudFlare; print CloudFlare.__file__'
~/proj/cloudflare/CloudFlare/__init__.py
$
$ cli4
usage: cli4 [-V|--version] [-h|--help] [-v|--verbose] [-q|--quiet] [-j|--json] [-y|--yaml] [-r|--raw] [-d|--dump] [--get|--patch|--post|--put|--delete] [item=value ...] /command...
$ which cli4
~/.local/bin/cli4

Or my preferred method, virtualenv. Then you have a clean environment that is easy to scratch. If you like this way of working you can look up virtualenvwrapper later to make it more convenient. Because the virtualenv is empty of Python packages installed globally or locally this is the only method with which one would have found the setup.py dependency issue. The install doesn't have to be editable.

$ pwd
~
$ virtualenv ~/venvs/cloudflare
Installing setuptools, pip, wheel...done.
$ source ~/venvs/cloudflare/bin/activate
(cloudflare) $
(cloudflare) $ pip install --editable proj/cloudflare
Successfully installed cloudflare future-0.16.0 logger-1.4 pyyaml-3.12 requests-2.12.4
(cloudflare) $
(cloudflare) $ which cli4
~/venvs/cloudflare/bin/cli4
(cloudflare) $
(cloudflare) $ pip freeze
-e git+git@github.com:yesbox/python-cloudflare.git@c0133052de575c2e277d51643b8c8d39bbf96fe3#egg=cloudflare
future==0.16.0
logger==1.4
PyYAML==3.12
requests==2.12.4
(cloudflare) $
(cloudflare) $ deactivate
$
$ cli4
-bash: cli4: command not found
$  rm -r ~/venvs/cloudflare

With any of the two latter methods your example scripts should also function without the "sys.path.insert(0, os.path.abspath('..'))" but that I have not tested.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yesbox @mahtin
Morty Proxy This is a proxified and sanitized view of the page, visit original site.