-
-
Notifications
You must be signed in to change notification settings - Fork 264
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
Packer v2 Rewrite #1042
base: master
Are you sure you want to change the base?
Packer v2 Rewrite #1042
Conversation
- Add tests where possible: testing a package manager can be a pain because of requirements to interact with the file system, etc. However, `packer`'s current tests do not cover much, and new code should at least strongly consider adding unit tests. | ||
- Avoid giant functions: `packer`'s logic contains some monstrous functions (e.g., `manage`, the main compilation logic, etc.) that are difficult to work with. New code should strive to use more, simpler functions in a more modular design. | ||
- Prioritize clean, performant code over total flexibility: `packer`'s design allows for a very flexible range of input formats. Although we don't want to completely lose this property, supporting all of these formats (and other use cases) has contributed to code bloat. If there's a choice to be made between losing some flexibility and significantly increasing the complexity of the code, seriously consider removing the complexity. | ||
- Reduce redundancy: `packer` currently has an annoying amount of redundant/overlapping functionality (the worst offenders are the `requires`/`wants`/`after` family of keywords, but there's also duplication of logic in the utilities, etc.). The rewrite should aim to reduce this. |
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.
Generally this looks great!
Regarding redundancy, there's a lot of code-duplication in packer.update
and packer.sync
. Maybe there are reasons for this but if things are structured well, it seems like sync
could just be something like:
packer.sync = function(..)
packer.clean()
packer.update(...)
packer.compile()
end
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.
That's the idea! I just haven't gotten to that part of the rewrite yet.
@wbthomason thanks for opening it, looks good 👍🏿. I was thinking I'd raise some PRs against this to help move it on, but will probably start with smaller things like adding Emmy Lua annotations to as many places as possible. I personally heavily rely on those when writing Lua, so would be nice if the types in the code base were clearer. I've also been using the lazy loading pattern popularized by TJ in https://github.com/tjdevries/lazy.nvim and thought I might add it here? It defers the requiring of modules till they are actually indexed, so you can require all modules at the top of a file, but they won't actually be required till they are used. You can then combine this with local lazy = require('packer.lazy')
--- @module 'packer.config'
local config = lazy.require('packer.config') Primarily because you've talked about reducing the cost of requiring packer, so this should help. |
Oh, brilliant! That looks quite useful. Separately, more annotations is also a great way to start - that's a stated goal of the rewrite and will make working with the codebase substantially nicer. Thanks! |
This is a WIP PR to complete the first stage of this
roadmap,
at least through the
load
function.Current progress:
The next steps are:
packer
module and your plugin specs.Contributions to this PR are welcome!