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

Fix datafile packaging issue #332

Merged
merged 1 commit into from
Jun 26, 2019
Merged

Fix datafile packaging issue #332

merged 1 commit into from
Jun 26, 2019

Conversation

shaunagm
Copy link
Contributor

@shaunagm shaunagm commented Jun 26, 2019

Fixes issue #279. Removes line include_package_data=True, in setup.py, an alternative method for including data files which wasn't working due to the relevant lines being commented out in manifest.in and which was conflicting with the more precise specification of which files we wanted. Also fixes the path for the SCFdata.csv file, which pointed to the wrong directory.

@shaunagm
Copy link
Contributor Author

@pkofod, when you get a chance can you confirm this fix works for you too? It should, but I want to sanity-check before pushing a release with this change. Instructions on how to test changes is in issue #279.

@pkofod
Copy link
Contributor

pkofod commented Jun 26, 2019

Thanks for the instructions in #279 let me just try it out.

@pkofod
Copy link
Contributor

pkofod commented Jun 26, 2019

Checked locally and it worked perfectly! Data files are now in the folders (unless there are some that are missing that I didn't know should be there. Let me suggest that we also test that they are there at some point using the testing infrastructure).

@shaunagm
Copy link
Contributor Author

Added a note to test for that, merging now.

@shaunagm shaunagm merged commit 4780d25 into master Jun 26, 2019
@pkofod
Copy link
Contributor

pkofod commented Jun 26, 2019

Good job! (seriously, these "oh, I guess I'm just going to vising 64% of the internet to find one line to change" are the worst!)

@llorracc
Copy link
Collaborator

@shaunagm, thanks for bird-dogging this!

But I'm not quite sure from the chain whether what you did was just to fix the infrastructural problem, and we still need to fix some code, or whether everything now is fixed. If the latter, did you move the files that were previously missing somewhere else, or just restored them to their original locations?

@shaunagm
Copy link
Contributor Author

Chris, the files were always in the repository, but an error in the setup.py file meant that when the repository was packaged for distribution, the files were left out. This is why when you install econ-ark using pip, the files aren't there, but when you clone the repository from github, they are.

My fix means that the next time we package the project - that is, the next time we do a release - that package will have the files. But the problem is not currently fixed in the sense that there's no recent release on pip that has the files, and the default release does not have the files.

I can make a new release tomorrow or Friday. The question is, is this a big enough bug that we want to do a new default release - 0.10.2 - just to include this change? Or are we okay with people missing the files for a little while, and releasing the change as 0.10.1.dev1?

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