Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load prototypes from JSON #30

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Load prototypes from JSON #30

wants to merge 3 commits into from

Conversation

bburky
Copy link
Collaborator

@bburky bburky commented Mar 17, 2015

Don't merge this yet. Let's leave this pull request open so we can have a discussion on the changes.

Load prototype definitions from JSON as discussed in #29.

Python only really installs packages. This moves prototypes.json into a hashid_data directory. And the directory needs a __init_.py to actually be a package.
@bburky
Copy link
Collaborator Author

bburky commented Mar 17, 2015

This first commit is just moving stuff around so that setup.py/pip can install the data.

Python only really installs packages. This moves prototypes.json into a hashid_data directory. And the directory needs a _init.py to actually be a package. (Python packaging drives me insane.)

I would actually rather make a hashid package. This would involve moving hashid.py into that directory and doing a bit of reorganizing. I'll try that later, but this was easy to do quickly.

I'll actually start work on loading the prototypes JSON in hashID now.

@bburky
Copy link
Collaborator Author

bburky commented Mar 17, 2015

Please test this on all platforms and installation methods you support. I tested it on Python 2.7.8 running from the repo directory and as a local installation. Package it however you usually do and maybe even upload it to testpypi. And test it on Python 3 please.

I just discovered pkgutil.get_data() which made this less much less terrible than I expected. Apparently it's in the standard library and lets me just ask for the contents of a file placed relatively to a module.

@psypanda
Copy link
Owner

I tested this on Python 3 and pkgutil.get_data() gives the following error:

Traceback (most recent call last):
  File "C:\Users\psypanda\Downloads\hashID-json\hashID-json\hashid.py", line 163, in <module>
    main()
  File "C:\Users\psypanda\Downloads\hashID-json\hashID-json\hashid.py", line 124, in main
    hashID = HashID()
  File "C:\Users\psypanda\Downloads\hashID-json\hashID-json\hashid.py", line 51, in __init__
    for prototype in json.loads(pkgutil.get_data('hashid_data', 'prototypes.json')):
  File "C:\Python34\lib\json\__init__.py", line 312, in loads
    s.__class__.__name__))
TypeError: the JSON object must be str, not 'bytes'

json accepts bytestrings in Python 2. In Python 3, the input must be Unicode string.
Or we use the JSON_PATH from f36444c instead of pkgutil?

I think we should reorganize and make a hashid package (as you suggested in your comment) and lay out our files somewhat like this:

hashID/
|-- doc/
|   |-- CHANGELOG
|   |-- HASHINFO.xlsx
|   |-- LICENSE
|-- .gitignore
|-- setup.py
|-- setup.cfg
|-- MANIFEST.in
|-- README.rst
|-- hashid/
|   |-- __init__.py
|   |-- hashid.py
|   |-- prototypes.json

The only thing I can't get to work is to install the prototypes.json file using setup.py. Neither data_files nor package_data seem to do the trick, or I am missing something essential 😞

@bburky
Copy link
Collaborator Author

bburky commented Mar 18, 2015

I tested this on Python 3 and pkgutil.get_data() gives the following error: [...]

Hmm. I think I just have to decode the Unicode encoding first. pkgutil.get_data() should be the best solution because it can find the data files no matter how strangely the package is installed (wheel, zip, whatever).

I will reintroduce JSON_PATH just to be a string constant though. So it's clear what the hard coded path is.

I think we should reorganize and make a hashid package (as you suggested in your comment) and lay out our files somewhat like this: [...]

Looks good. We may actually want hashid.py to become the __init__.py under hashid/ though. The structure you currently have will require people to do import hashid.hashid if they use hashid as a Python module.

The only thing I can't get to work is to install the prototypes.json file using setup.py. Neither data_files nor package_data seem to do the trick, or I am missing something essential 😞

And you listed the file in Manifest.in? Not sure what it would be then.

@psypanda
Copy link
Owner

Looks good. We may actually want hashid.py to become the init.py under hashid/ though. The structure you currently have will require people to do import hashid.hashid if they use hashid as a Python module.

You are right, I didn't think of this, thanks for the heads up!

I think I found my mistake with the packaging, will try again after we restructured the files.

@bburky
Copy link
Collaborator Author

bburky commented Mar 29, 2015

First try at moving everything into a hashid module. This moves hashid.py to hashid/__init__.py and also moves prototypes.json into the same module directory.

However a few errors are shown when I do a pip install . to test the installation.

Processing /Users/blake/Code/hashID
    /usr/local/lib/python2.7/site-packages/setuptools-12.0.5-py2.7.egg/setuptools/dist.py:283: UserWarning: The version specified requires normalization, consider using '3.2.0.dev0' instead of '3.2.0-dev'.
    file hashid.py (for module hashid) not found
Installing collected packages: hashID
  Running setup.py install for hashID
    /usr/local/lib/python2.7/site-packages/setuptools-12.0.5-py2.7.egg/setuptools/dist.py:283: UserWarning: The version specified requires normalization, consider using '3.2.0.dev0' instead of '3.2.0-dev'.
    file hashid.py (for module hashid) not found
    file hashid.py (for module hashid) not found
    file hashid.py (for module hashid) not found
    Installing hashid script to /usr/local/bin
    file hashid.py (for module hashid) not found
Successfully installed hashID-3.2.0.dev0

...Except everything seems to work fine and it actually installs correctly. Running the command line hashid tool works and I can import hashid in a python console after installing. All the files seemed to be installed correctly and in the right places too.

I tried my best to fix up setup.py for the new structure, but I don't know what's the source of the file not found messages during installation.

@psypanda
Copy link
Owner

Processing /Users/blake/Code/hashID
    /usr/local/lib/python2.7/site-packages/setuptools-12.0.5-py2.7.egg/setuptools/dist.py:283: UserWarning: The version specified requires normalization, consider using '3.2.0.dev0' instead of '3.2.0-dev'.
    file hashid.py (for module hashid) not found
Installing collected packages: hashID
  Running setup.py install for hashID
    /usr/local/lib/python2.7/site-packages/setuptools-12.0.5-py2.7.egg/setuptools/dist.py:283: UserWarning: The version specified requires normalization, consider using '3.2.0.dev0' instead of '3.2.0-dev'.
    file hashid.py (for module hashid) not found
    file hashid.py (for module hashid) not found
    file hashid.py (for module hashid) not found
    Installing hashid script to /usr/local/bin
    file hashid.py (for module hashid) not found
Successfully installed hashID-3.2.0.dev0

This error happens because of py_modules=['hashid'] in setup.py.
By simply removing py_modules pip install . doesn't throw any error.
I have no idea if we need py_modules at all - Still reading through documentation. 😕

But when I run __init.py__ without installing it via pip I' am getting the following errors:
Python 2.7.8:

psypanda@xubuntu:~/Desktop/hashID-json$ python2 ./hashid/__init__.py 
Traceback (most recent call last):
  File "./hashid/__init__.py", line 163, in <module>
    main()
  File "./hashid/__init__.py", line 124, in main
    hashID = HashID()
  File "./hashid/__init__.py", line 51, in __init__
    for prototype in json.loads(pkgutil.get_data('hashid', 'prototypes.json')):
  File "/usr/lib/python2.7/json/__init__.py", line 338, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python2.7/json/decoder.py", line 366, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
TypeError: expected string or buffer

Python 3.4.2:

psypanda@xubuntu:~/Desktop/hashID-json$ python3 ./hashid/__init__.py 
Traceback (most recent call last):
  File "./hashid/__init__.py", line 163, in <module>
    main()
  File "./hashid/__init__.py", line 124, in main
    hashID = HashID()
  File "./hashid/__init__.py", line 51, in __init__
    for prototype in json.loads(pkgutil.get_data('hashid', 'prototypes.json')):
  File "/usr/lib/python3.4/json/__init__.py", line 312, in loads
    s.__class__.__name__))
TypeError: the JSON object must be str, not 'NoneType'

@bburky
Copy link
Collaborator Author

bburky commented Mar 30, 2015

😧

OK, I'll look at it some more. Both of your errors seem to be because it can't load the package data file for some reason. Does it work fine when use the version installed into your PATH via pip?

Also, I still haven't fixed the byte string stuff for python3 yet. So it isn't expected to work yet.

@psypanda
Copy link
Owner

Does it work fine when use the version installed into your PATH via pip?

Yes it does work fine when I install it via pip.

I also haven't figured out yet why it doesn't load the package data. 😞

@bburky
Copy link
Collaborator Author

bburky commented Mar 30, 2015

Ok. Good.

Also swear I tested it from the command line like that too. It may matter what directory you run it from?

Considering pip/setup.py is now the reccomended installation method, this doesn't matter too much, right? It just makes testing and development a problem. (Though you can already just use pip install -e . for an editable testing installation instead.)

@@ -39,14 +39,15 @@ def get_version(*file_paths):
setup(
name='hashID',
packages=find_packages(exclude=['HASHINFO.xlsx']),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a mistake here excluding HASHINFO.xlsx, this is not needed at all - the line should be just
packages=find_packages(),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants