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

When bumping the galaxy metapackage, also bump its galaxy-* deps and include/update pinned-requirements.txt #23

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

natefoo
Copy link
Member

@natefoo natefoo commented Dec 6, 2024

for line in content:
if line.startswith("version = "):
line = f"version = {new_version}"
elif line.startswith("install_requires ="):
Copy link
Member

Choose a reason for hiding this comment

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

This seems kind of weird to execute on all packages. Do you need to first determine if this is the meta package ?

Copy link
Member

Choose a reason for hiding this comment

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

(Also any reason we don't symlink the requirements file ?)

Copy link
Member

Choose a reason for hiding this comment

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

Also, can we have this in a separate function? A function called bump_package_version shouldn't be overwriting package requirements, I think. Besides, we call it twice, and we don't want to have to do this new work twice (or each time we need to update the version number of a package). No to mention ease of testing..

Copy link
Member Author

Choose a reason for hiding this comment

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

(Also any reason we don't symlink the requirements file ?)

Setuptools does not use this automatically, does it? Apparently since 62.6 you can specify it in setup.cfg with:

install_requires = file: requirements.txt

but this can't be combined with the list syntax we need to specify the galaxy-* subpackages (and the --index-url is invalid for setuptools as well).

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, can we have this in a separate function?

Sure. I did it here because we were already modifying the file here, so you know, didn't want to incur the performance penalty of reading/writing a 1 KB file twice 😆 but agree it's not in scope for the function.

@jdavcs
Copy link
Member

jdavcs commented Dec 14, 2024

Please don't merge before #25. I can help with fixing the conflicts.

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.

3 participants