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 Dash/Underscore Issue in Package Name #31

Merged
merged 2 commits into from
Aug 23, 2023
Merged

Conversation

michaelweinold
Copy link
Contributor

In the utility function

def get_version_tuple() -> tuple:
    """Returns version as (major, minor, micro)."""

    def as_integer(version_str: str) -> Union[int, str]:
        try:
            return int(version_str)
        except ValueError:
            return version_str

    return tuple(
        as_integer(v)
        for v in importlib.metadata.version("bw-processing")
        .strip()
        .split(".")
    )

having either bw-processing or bw_processing in the source code of the utility function works exactly the same (tested locally on macOS):

>>> import bw_processing as bp
>>> bp.utils.get_version_tuple()
(0, 8, 3)

However, it is possible that this is not the case during the build process of the JupyterLite site (cf. emscripten-forge/recipes#616).

@cmutel changed the distribution name of the package to bw-processing in September 2022 (changelog entry).

My guess is that this fix might resolve the empack build issues we've been experiencing.

@michaelweinold michaelweinold added bug Something isn't working help wanted Extra attention is needed labels Aug 23, 2023
@cmutel
Copy link
Member

cmutel commented Aug 23, 2023

@michaelweinold Could you please also add a test to make sure that the result of get_version_tuple is a tuple? I would like to make sure this is working in all the CI systems before merging.

BTW, this is an excellent catch, I think we need to do it then for other BW libraries which (could) have hyphens.

I have lost so much time dealing with this ********, it is beyond my understanding why pypi will change the distribution name of a library you are uploading on it's own, and it has caused big issues on Windows as well. And some people defend it!? Honestly in the future I think just avoiding underscores or hyphens in library names is the only sensible course of action.

@michaelweinold
Copy link
Contributor Author

...I added a test function to tests/test_utils.py. One thing I'm not sure about is how bp.utils.get_version_tuple() can work (on macOS) with either version. Any ideas? Intuitively this might be an issue for the reliability of the test function.

@cmutel
Copy link
Member

cmutel commented Aug 23, 2023

Merging this. If it is still an issue, we can specify the version in setup.cfg as linked above, though I don't know how this works without importing the actual library and executing Python code, which was the whole point of putting the version in a separate file...

@cmutel cmutel merged commit eaad673 into main Aug 23, 2023
12 checks passed
@cmutel cmutel deleted the version-information-fix branch August 23, 2023 19:46
@cmutel
Copy link
Member

cmutel commented Aug 24, 2023

One thing I'm not sure about is how bp.utils.get_version_tuple() can work (on macOS) with either version. Any ideas? Intuitively this might be an issue for the reliability of the test function.

I don't think we need to worry about how it works, setuptools or whatever in the packaging infrastructure does magical transformations which treat hyphens and underscores as exactly the same (but only sometimes, of course). If the tests pass then it does work, that's enough.

From what I can tell from past experience, the key is to make sure the first package uploaded to pypi only uses underscores, also in the distribution and file name. Then nothing is ever changed and things are fine. The problem here was the rule wasn't followed at the very beginning, and the only way to fix it would be to delete the pypi record completely, and even then I am not sure that it would behave the way we want when re-created.

Let's hope this PR was enough!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants