Skip to content

Rename package to epidatpy #15

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

Merged
merged 5 commits into from
Aug 10, 2022
Merged

Rename package to epidatpy #15

merged 5 commits into from
Aug 10, 2022

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Aug 5, 2022

Closes #13.

Replaced all instances of delphi_epidata with epidatpy in

  • source
  • documentation
  • tests
  • installation files

Can import package and use it. CI should pass.

@dshemetov dshemetov mentioned this pull request Aug 5, 2022
5 tasks
@dshemetov dshemetov requested a review from brookslogan August 8, 2022 23:24
@brookslogan
Copy link

My familiarity with Python is limited and stale, so please bear with me. I'm testing out the first README.md commands, encountered trouble. For the first one:

python -m venv ../venv-epidatpy
../venv-epidatpy/bin/pip install --upgrade pip
#> [output elided]
../venv-epidatpy/bin/python --version
#> Python 3.10.5
../venv-epidatpy/bin/pip --version
#> pip 22.2.2 from /home/fullname/files/tooling/venv-epidatpy/lib64/python3.10/site-packages/pip (python 3.10)
../venv-epidatpy/bin/pip install -e "git+https://github.com/cmu-delphi/epidatpy.git"
#> ERROR: Could not detect requirement name for 'git+https://github.com/cmu-delphi/epidatpy.git', please specify one with #egg=your_package_name

and the second one (for the pre-rename version) also has the same type of message.

@brookslogan
Copy link

Is this an issue with my configuration, or does it mean that the install string needs an egg setting suffix?

@brookslogan
Copy link

  • Changes that were made look fine to me.
  • Potentially missing changes?
    • README.md: [pypi-image], [pypi-url], [docs-image], [docs-url]
    • index.rst: 4x references to "delphi-epidata". One is straightforward... link to github should be to epidatpy. The pypi links and installation commands... I assume that these are referring to the old client, right? And that we are making a new pypi package for the new one?

* install command updated with #egg param
* fixed package references in index.rst
@dshemetov
Copy link
Contributor Author

dshemetov commented Aug 9, 2022

So the current docs (README + index.rst) are written as if this package was already on PyPI and all the documentation sites existed. I figure we will make those before release, so the documentation is accurate.

Otherwise, I addressed your comments: README is updated, index.rst is updated, and I added an #egg# parameter (I thought you could get away without it, but not in pip with the -e flag).

@brookslogan
Copy link

Some notes:

  • In index.rst: https://cmu-delphi.github.io/delphi-epidata/api is a broken link, but not due to a missing rename -> epidatpy. We must have broken it via updates to the API docs, outside this repo. So might want to file a separate issue.
  • The first install command still fails for me, but I assume that merging this PR will fix it.
  • The second install command seems to succeed for me both with #egg=delphi_epidata and #egg=delphi-epidata, but I can only pip uninstall delphi-epidata, not pip uninstall delphi_epidata. I don't know enough about Python packge dev to know if this is an issue. I assume since it seems to install successfully in either case, and we're migrating away, this isn't something to worry about.
  • File diffs look good.

I think this looks fine to merge, but leave hitting the button to you, in case one of the above looks like it actually requires changes.

@dshemetov
Copy link
Contributor Author

dshemetov commented Aug 10, 2022

Good catch on the broken link! I'll fix and the first line should work.

As for your third point, this is just one of those strange Python things, where the PyPI distribution name is delphi-epidata and that's what you call with pip, but delphi_epidata is the actual package name. So uninstalling should reference the first. I guess that #egg=delphi_epidata works because the package folder is delphi_epidata. Not sure why #egg=delphi-epidata works, since that's not anywhere in the package metadata 🤷 .

@dshemetov dshemetov merged commit 8659609 into dev Aug 10, 2022
@dshemetov dshemetov deleted the ds/rename-package branch August 10, 2022 18:18
@brookslogan
Copy link

First install command does indeed appear to work after the merge!

Thanks; I couldn't quickly find anything explaining which of the several names the #egg thing should use. Somehow within the top level of the repo directory, I have an untracked src folder containing a single subdirectory delphi-epidata with delphi_epidata-before-rename checked out. Not sure if that's related to whatever thing made #egg=delphi-epidata work.

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.

Rename package to epidatpy
2 participants