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

Encapsulating package managers #4918

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

foarsitter
Copy link
Collaborator

@Andrew-Chen-Wang, @luzfcb & @browniebroke I would like to have your opinion about this draft.

In this PR I'm trying to encapsulate pip so we can implement a variety of package managers.

One of the problems I'm experiencing is to determine which package manager is selected. I solved this by passing the cookiecutter variable to the install extension but it clutters the DX imho. {{ "ruff"|install}} is looking a way better then {{ "ruff"|install(cookiecutter)}}.

What do you think about this approach?

Feel free to fork or push to this branch.

Copy link
Contributor

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

I don't have a particular opinion around the DX of cookiecutter django itself nor am I an expert in cookiecutter. I will say that when something is done repeatedly and consistently as shown in the PR, it's not too hard to find and replace. The key is find and replace should be easy and not miss anything. Something that might work better is chaining the filters instead like

{{ install(cookiecutter, "development") }}

maybe this syntax would imply development as something more varying? Tbh it shouldn't matter and the code to me looks like it suffices:)

Thanks for the draft!



class UV(PackageManager):
"""Poetry package manager"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be out of scope apologies: this can be the exact same as pip install section except you would prepend pip with uv like uv pip install etc.

Regarding lock files, you should be able to run uv pip compile requirements.txt -o requirements-lock.txt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So UV can extend PIP just by prepending the command with uv?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not feature complete I think, but for the basics, like pip install -r which is the extent of 99% of projects, yes, prepending uv suffices.

Copy link
Member

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

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

At first glance, my gut reaction is that it's "too clever" 😄:

  • In the template, one may ask "whare is this install function coming from"?
  • If we use extensions for the package manager, are we going to start using it for other options as well? Would that scale after we have 5 or 10 extensions?
  • in the template, it can be tricky to map out what each parameter combinations will render to... But maybe that's just me not being used to it.

Some of these issues are very much fixable with documentation or a better API, which we can work out, and I appreciate you opening this to collect feedback early.

I'm going to play devil's advocate for a bit... Abstractions are great until they break, and I would challenge how much this one is needed (just to make sure we actually need it).

Would it be simpler to read with a few if/else? What does this solution offer that we if/else?

@@ -21,7 +21,7 @@ steps:
path: ${PRE_COMMIT_HOME}
commands:
- export PRE_COMMIT_HOME=$CI_PROJECT_DIR/.cache/pre-commit
- pip install -q pre-commit
- {{ "-q pre-commit"|install(cookiecutter)}}
Copy link
Member

Choose a reason for hiding this comment

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

So that's rendered out to either pip install, or the package manager equivalent, when the template is rendered?

@browniebroke
Copy link
Member

Something that might work better is chaining the filters instead like

{{ install(cookiecutter, "development") }}

That's a good idea, I think that looks nicer... Going one step further, can we do something like this:

# install dev deps
{{ install(cookiecutter, dev=True) }}

# install prod
{{ install(cookiecutter, prod=True) }}

# install package(s)
{{ install(cookiecutter, "ruff", "flake8") }}

After my initial pushback, I think the idea is growing on me 😄

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.

None yet

3 participants