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

Data packaging and Directory Restructuring #6

Merged
merged 3 commits into from
Oct 11, 2016

Conversation

nickdelgrosso
Copy link
Contributor

Cool package! However, when I "pip installed" dfply, the diamonds.csv flie didn't download as well, and I got import errors, since the file gets read in as part of the package load process. I took a look at the packaging, and tried to help out by making some changes that would make it more pip-compatible and would (I hope--I'm still a newbie at packaging, and this is actually my first pull request ever!) simplify the growth of the project by making "six" and "pandas_ply" explicit global requirements that could be downloaded during installation.

This pull request:

Moves data out of python package,
added setup.py and MANIFEST.in links to data
made data.py a module that loads the data, to keep code working without API change.
Added pandas_ply and six to requirements, to remove vendor code from python package as well.
I'm sorry that I made all these into a single commit (stupid, I know). If there are any fixes you'd like me to make, please let me know!

…s to data, and made data.py a module that loads the data, to keep code working without API change. Added pandas_ply and six to requirements, to remove vendor code from python package as well.
@kieferk
Copy link
Owner

kieferk commented Oct 10, 2016

This sounds like a good idea to me. Sorry about the data issue with pip, I was not aware of that.

I probably won't be able to merge this in until later on this evening or tomorrow morning, but will get to it as soon as I can. I am going to change the target branch to develop from master to make sure everything plays nice with the changes, then I'll merge it into the master branch as the next version.

@kieferk kieferk changed the base branch from master to develop October 11, 2016 02:56
@kieferk kieferk merged commit 3b78793 into kieferk:develop Oct 11, 2016
@kieferk
Copy link
Owner

kieferk commented Oct 11, 2016

OK everything is checked out and merged into master branch as well as develop. Thanks for doing this!

@nickdelgrosso
Copy link
Contributor Author

Hey, that's great! Glad I could contribute--this is a really cool
project!

Best wishes,

Nick

On 2016-10-11 05:17, Kiefer Katovich wrote:

OK everything is checked out and merged into master branch as well as
develop. Thanks for doing this!

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub [1], or mute the
thread [2].

Links:

[1] #6 (comment)
[2]
https://github.com/notifications/unsubscribe-auth/ADoHd0mLIBM5ikJAxHmEmCp5Ti1G4vz0ks5qyv-0gaJpZM4KTD8x

@bmwilly
Copy link

bmwilly commented Oct 21, 2016

Hi. Cool package indeed! I tried installing with

pip install git+https://github.com/kieferk/dfply.git

and still got the error you describe above when trying to import (diamonds.csv does not exist). Any ideas?

@nickdelgrosso
Copy link
Contributor Author

Okay, good to know; it sounds like the data isn't being packaged correctly. Really, diamonds.csv shouldn't be read during the import in the first place, so maybe instead of improving the packaging, it would make more sense to change that aspect. Meanwhile, @bmwilly could you please try downloading the package using git and installing it via the setup.py package? I just tried it on my work computer and it worked out fine. If you're not familiar with the process, the commands would look something like this::

git clone https://github.com/kieferk/dfply.git
cd dfply
python setup.py install

@bmwilly
Copy link

bmwilly commented Oct 21, 2016

@neuroneuro15 yes, that works!

@kieferk
Copy link
Owner

kieferk commented Oct 25, 2016

Hi all,

Sorry to hear this is not working via pip. I try and figure out how to fix
the csv packaging once github is back online.

On Fri, Oct 21, 2016 at 8:28 AM, Brandon Williams notifications@github.com
wrote:

@neuroneuro15 https://github.com/neuroneuro15 yes, that works!


You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#6 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ARNAmFAR8yecyTBUTYh613kMRGB2xoXeks5q2NoKgaJpZM4KTD8x
.

@bmwilly
Copy link

bmwilly commented Oct 25, 2016

@kieferk I think this was fixed already. I posted my comment before #8 was resolved (I'm not sure what commit actually fixed the problem).

@kieferk
Copy link
Owner

kieferk commented Oct 26, 2016

I think the github version was working but not the pip version. My re-org of the files was a fix for pip.

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.

3 participants