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

Update package names #208

Merged
merged 11 commits into from
Sep 21, 2021
Merged

Update package names #208

merged 11 commits into from
Sep 21, 2021

Conversation

CasperWA
Copy link
Contributor

@CasperWA CasperWA commented Sep 14, 2021

Update package names according to discussion in #205. Closes #205.

Note, this is a draft PR and only here to have CI running and an easy way of getting to the working branch.

Important: This PR now also updates the pytest testing, removing the unittest runner file and instead encapsulating all the test files in appropriate functions, which is the normal way to run pytest tests.
If this is an issue due to some developer not running the tests via pytest locally, or the unittest runner class in the test_runner.py file did extra checks and tests that are currently not included, this can either be extracted out of this PR or I can try to re-add it in a pytest-friendly way.

As a note, this update/edit of the test files is not a rewrite, and the tests should be properly ported to the pytest test-writing architecture in another PR (creating fixtures, etc. as needed).

@CasperWA CasperWA marked this pull request as ready for review September 15, 2021 13:29
@CasperWA CasperWA marked this pull request as draft September 15, 2021 13:30
@CasperWA CasperWA force-pushed the cwa/close-205-package-name branch from e526b58 to 6e838f5 Compare September 17, 2021 09:37
@CasperWA CasperWA marked this pull request as ready for review September 17, 2021 09:44
Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

I think this looks good and did not find any glaring things to fix. I have one question though: since we decided to use ontopy this could come in conflict with the python package ontopy if someone would want to use that at the same time. Is this an issue we should care about?

@CasperWA
Copy link
Contributor Author

CasperWA commented Sep 21, 2021

(..) I have one question though: since we decided to use ontopy this could come in conflict with the python package ontopy if someone would want to use that at the same time. Is this an issue we should care about?

This is an excellent (a difficult to answer) question.

We can extrapolate that noone is using the old ontopy package (PyPI package, GitHub repository), due to the last commit being done almost 12 years ago. Furthermore, it is not installable in Python3, so the package wouldn't even show up if using pip in Python3 (at least this is my experience with packages that remove support for older versions).

However, it doesn't, of course, change the fact that the project could be taken up again at any time and be reused.
I guess we have 3 options going forward (as I see it):

  1. Do nothing - with the current lay of the land, I don't see this as bad an option as it sounds.
  2. Rename our module to be different from ontopy.
  3. Try to "transfer" the package name on PyPI to us or simply reach out to the original author (for more info on the first part, see here).

This still cannot be updated for GitHub links and the Dockerfile.
While the first is obvious (the repository still has not been renamed),
the latter is less so. The main reason here is that the Docker
instructions outline how one installs the package in within the Docker
container. And the `setup.py` file instructs to find data files in the
shared folder `EMMO-python`.
@francescalb
Copy link
Collaborator

I agree, conflict with the older ontopy from 2009 is not a problem. We should consider reaching out to get the ontopy anyways, to make sure that noone else does it and makes a conflicting new ontopy. Bur for now I think we are good.

@francescalb
Copy link
Collaborator

Another issue is the documentation. Now it says pip install EMMO in the readme, but should be pip install EMMOntoPy. And there should be a small section about the name change in the README.

@CasperWA
Copy link
Contributor Author

Another issue is the documentation. Now it says pip install EMMO in the readme, but should be pip install EMMOntoPy. And there should be a small section about the name change in the README.

Yes and no, I guess. It's the same "chicken'n'egg" problem as changing all occurrences of EMMO-python to EMMOntoPy. Since the repository has not yet changed name (if it should), then these updated references don't make sense yet. A draft PR should be created that change these though and be merged as soon as the repo name has been updated.

For the installable EMMO package, it's similar. Since the README references the EMMO package on PyPI, which as of now, still hasn't been updated to be EMMOntoPy, then the reference makes no sense - yet. But similarly, a draft PR should be made to update this as soon as a new release with the new name has been made.

Copy link
Collaborator

@francescalb francescalb left a comment

Choose a reason for hiding this comment

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

I am convinced. Looks nice :)

@CasperWA CasperWA merged commit 95aba42 into master Sep 21, 2021
@CasperWA CasperWA deleted the cwa/close-205-package-name branch September 21, 2021 09:04
@CasperWA CasperWA mentioned this pull request Sep 21, 2021
CasperWA added a commit that referenced this pull request Sep 21, 2021
This was already done once for the #217 PR, however, due to the
remodelling of the repository in #208 this change seems to have
disappeared.
CasperWA added a commit that referenced this pull request Sep 22, 2021
This was already done once for the #217 PR, however, due to the
remodelling of the repository in #208 this change seems to have
disappeared.
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.

change package name
2 participants