brightway-lca / bw_processing

Tools to create structured arrays in a common format
https://docs.brightway.dev/projects/bw-processing/
BSD 3-Clause "New" or "Revised" License
7 stars 5 forks source link

Fix Dash/Underscore Issue in Package Name #31

Closed michaelweinold closed 1 year ago

michaelweinold commented 1 year ago

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. https://github.com/emscripten-forge/recipes/pull/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.

cmutel commented 1 year ago

@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 commented 1 year ago

...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 commented 1 year ago

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 commented 1 year ago

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!