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

Convert to a Python package #16

Merged
merged 13 commits into from Apr 6, 2020
Merged

Convert to a Python package #16

merged 13 commits into from Apr 6, 2020

Conversation

khaeru
Copy link
Contributor

@khaeru khaeru commented Apr 4, 2020

Excepting tests and helper code for us devs, the package consists of two lines that define one object:

from iam_units import registry

How to review:

  • Will merging this PR break any existing code? If not, then let's handle any adjustments in additional PRs to keep things quick and tidy.
  • The name 'units' is taken on PyPI: https://pypi.org/project/units/, so I went with 'iam_units'. Good? I could also live with 'ia_units'.

@khaeru khaeru linked an issue Apr 4, 2020 that may be closed by this pull request
@gidden
Copy link
Member

gidden commented Apr 5, 2020

@khaeru took a look and all seems very immaculate. @znicholls, FYI that this is now installable. @danielhuppmann I guess we can do a quick patch on pyam to depend on this?

Also, very happy to see that setup.py now looks like it's actually easy to implement instead of the complicated nonsense I've had to work with for the past 10 years!

@gidden
Copy link
Member

gidden commented Apr 5, 2020

Ah, I forgot to mention. We use pyam-iamc name on PyPI since pyam was taken. That could be one pattern to follow here.

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Minor comments on the copyright holders, update-script and fixing a broken link to the new pyam-docs

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
iam_units/update.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@danielhuppmann
Copy link
Member

@danielhuppmann I guess we can do a quick patch on pyam to depend on this?

Yes, @dependabot proves quite useful here. But unless this is also released via conda and/or pip, I don't think we'll get around the submodule hassle (but that's a minor hassle now that it's set up).

@danielhuppmann
Copy link
Member

We use pyam-iamc name on PyPI since pyam was taken. That could be one pattern to follow here.

-1. Don't think that units-iamc has a nice ring to it...

@znicholls
Copy link

znicholls commented Apr 5, 2020

@znicholls, FYI that this is now installable

Thanks @gidden and thanks @danielhuppmann for your comments about authorship. Both appreciated and this looks like a very easy maintenance way of getting the behaviour you need.

For completeness, we needed a units repository in silicone which had wide gas and gwp context coverage so I've actually already split out https://github.com/openscm/openscm-units from https://github.com/openscm/scmdata. I didn't raise this here as I thought the discussions in #9 suggested a package wasn't desired. I'm happy to work out a way to combine openscm-units and this repo into a shared resource if there's desire. Also fine to keep them separate for the time being though (openscm-units certainly doesn't have the feature of being 'as low-maintenance as possible' because of its desire to include more features).

But unless this is also released via conda and/or pip, I don't think we'll get around the submodule hassle

+1 from me, you can't release a package on PyPI that has url based install dependencies. Hence if you want pyam to depend on this, you'll need to make this pip installable too or stick with the submodule stuff. If you want an automated deployment workflow for this repo, the jobs from here onwards might be helpful (you need to set the right secrets, see details here and alter hard-coded names but then you're done).

Copy link
Member

@gidden gidden left a comment

Choose a reason for hiding this comment

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

LGTM - vote then for iam-units as suggested by @khaeru

Co-Authored-By: Daniel Huppmann <dh@dergelbesalon.at>
@khaeru
Copy link
Contributor Author

khaeru commented Apr 6, 2020

Sorry @danielhuppmann @gidden —one additional point based on #16 (comment): since this is now no longer simply content (a text file) but code, I will change the license to GPL3. Good?

@znicholls thanks for the pointer to the GitHub workflow examples and information about the other package.

@danielhuppmann
Copy link
Member

Sorry @danielhuppmann @gidden —one additional point based on #16 (comment): since this is now no longer simply content (a text file) but code, I will change the license to GPL3. Good?

Fine for me!

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

great work @khaeru, thanks for the effort! Approving, though I feel the list of authors should still be changed - see inline comment.

AUTHORS Outdated Show resolved Hide resolved
Co-Authored-By: Daniel Huppmann <dh@dergelbesalon.at>
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.

Establish a Python package
4 participants