Closed Iron-E closed 1 year ago
I am going to work on the pin animation, we might have conflicts, try to focus this PR on the essentials.
@romgrk I just pushed an update that removed PascalCase imports, and changed the name of the utils.hl
file.
Should the pin animations be based on this PR? It would be easier to work on #453 and #454 with this as a base for me as well
I just pushed an update that removed PascalCase imports, and changed the name of the utils.hl file.
I still see some imports with the PascalCase
.
Should the pin animations be based on this PR? It would be easier to work on https://github.com/romgrk/barbar.nvim/issues/453 and https://github.com/romgrk/barbar.nvim/issues/454 with this as a base for me as well
imho this PR has a lot of noise for the benefits that it provides. The only changes I'm happy to see are the highlight split and the fs functions. I'd rather do linting changes after we're done with the more complex ongoing changes.
I still see some imports with the PascalCase.
Oops, forgot to force push my last change. Done :)
imho this PR has a lot of noise for the benefits that it provides
Can you elaborate on why this seems like noise? It's all for the sake of trying to make things more consistent:
snake_case
for imports (https://github.com/romgrk/barbar.nvim/pull/451#discussion_r1161990900; imports had mixed casing without semantic reason)require
(#430; makes them easier to read)utils
into list
and table
, since:
require
s both, and list
has specific imports that it needs.local notify = utils.notify
) but not others.I'd rather do linting changes after we're done with the more complex ongoing changes.
We do have a precedent of making maintenance changes before shaking things up (e.g. #418), because it makes further changes easier to make
As I mentionned in formatting rules, I'd rather we incrementally do linting changes as we touch the code but it's really not a priority.
The noise also comes from things like local notify = utils.notify
. Sometimes having the module+function makes things easier to read, e.g. jump_mode.get_letter(...)
tells me more than just get_letter(...)
, so I'm not sure we should 100% of the time unwrap everything at the top. OTOH, I don't like seeing vim.api.nvim_win_get_width(...)
because the noise makes it less readable than just win_get_width(...)
.
For now, could you revert the changes in render.lua
? I'm refactoring in #455 so it's going to conflict.
For the rest of the changes, they're already done so we might as well merge them.
Unfortunately the render code is tied in with it, because it also uses the split utils
and had the mixed naming conventions (which is why I'd hoped we could base new code off of this, since there's going to be a lot of conflicts otherwise).
Either way, I've been using this for a bit so we can merge if you give the OK. If you want to wait until after your PR that's understandable, though I think applying #455 after this one is probably better since you're more familiar with the fixes that need to get patched in.
Just let me know— don't want to mess up what you're working on :)
This PR is based on #446, so its diff will look better after that merges.
A few things here, which I think will help maintenance in the long term:
utils
. It was a large module with several distinct groups of functions, all put together.utils.hl
has been moved to a new module,utils.Hl
.utils
.utils
methods operating onany[]
were moved toutils.List
.utils
to accessList.index_of
, orList.slice_from_end
, and there were imports inutils
only existing to implement these functions.utils
methods operating on{[string]: any}
were moved toutils.Table
for the same reason as theutils.List
split.utils.basename
, etc were moved tobarbar.Fs
.utils
just for implementing these methods.)Foo
, but only uses it forFoo.bar
, importbar
instead.Foo
forbar
when possiblebar
is a table, sinceFoo
might reassignbar
and the import would become stale.require
s as per #430