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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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""" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this 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)}} |
There was a problem hiding this comment.
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?
That's a good idea, I think that looks nicer... Going one step further, can we do something like this:
After my initial pushback, I think the idea is growing on me 😄 |
@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.