Skip to content

ref: granularize imports#451

Merged
Iron-E merged 5 commits into
masterfrom
ref/module-naming
Apr 28, 2023
Merged

ref: granularize imports#451
Iron-E merged 5 commits into
masterfrom
ref/module-naming

Conversation

@Iron-E

@Iron-E Iron-E commented Apr 9, 2023

Copy link
Copy Markdown
Collaborator

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:

  1. Split up 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.
      • There was a whole caching system for highlight definitions, and it crowded the other utils.
    • utils methods operating on any[] were moved to utils.List.
      • Many modules only needed to import utils to access List.index_of, or List.slice_from_end, and there were imports in utils only existing to implement these functions.
    • utils methods operating on {[string]: any} were moved to utils.Table for the same reason as the utils.List split.
    • utils.basename, etc were moved to barbar.Fs.
      • We have a whole module for functions related to getting information about the file system, so it probably belongs there.
      • (Plus, like before, there were imports in utils just for implementing these methods.)
  2. If a module imports Foo, but only uses it for Foo.bar, import bar instead.
    • This avoids doing repeated hashtable lookups on Foo for bar when possible
    • This cannot be done in the case where bar is a table, since Foo might reassign bar and the import would become stale.
  3. Put parens around requires as per Formatting rules #430
  4. Make all imports snake_case

@Iron-E Iron-E added the refactor Misc. change in an existing component of barbar.nvim label Apr 9, 2023
@Iron-E Iron-E added this to the 1.6 milestone Apr 9, 2023
@Iron-E Iron-E self-assigned this Apr 9, 2023
@Iron-E Iron-E force-pushed the ref/module-naming branch 6 times, most recently from 01e6709 to cb2e720 Compare April 10, 2023 04:29
Comment thread lua/barbar.lua Outdated
Comment thread lua/barbar/utils/hl.lua
Comment thread lua/barbar/events.lua Outdated
@Iron-E Iron-E force-pushed the ref/module-naming branch from cb2e720 to f501e48 Compare April 10, 2023 19:03
@romgrk

romgrk commented Apr 10, 2023

Copy link
Copy Markdown
Owner

I am going to work on the pin animation, we might have conflicts, try to focus this PR on the essentials.

@Iron-E Iron-E force-pushed the ref/module-naming branch from f501e48 to b9ef94b Compare April 10, 2023 19:41
@Iron-E

Iron-E commented Apr 10, 2023

Copy link
Copy Markdown
Collaborator Author

@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

@romgrk

romgrk commented Apr 10, 2023

Copy link
Copy Markdown
Owner

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 #453 and #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.

@Iron-E Iron-E force-pushed the ref/module-naming branch from b9ef94b to dab0819 Compare April 10, 2023 20:18
@Iron-E

Iron-E commented Apr 10, 2023

Copy link
Copy Markdown
Collaborator Author

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:

  • Convert to snake_case for imports (ref: granularize imports #451 (comment); imports had mixed casing without semantic reason)
  • Prevent future LSP type hint false reports (ref: granularize imports #451 (comment))
  • Add parens around require (Formatting rules #430; makes them easier to read)
  • Split utils into list and table, since:
    • there's no module with requires both, and
    • list has specific imports that it needs.
  • Get rid of recurring hashtable lookups where possible. We already did this in places (e.g. 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

@romgrk

romgrk commented Apr 10, 2023

Copy link
Copy Markdown
Owner

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.

@Iron-E Iron-E force-pushed the ref/module-naming branch 4 times, most recently from 68840d1 to b3d3efc Compare April 14, 2023 17:51
@Iron-E

Iron-E commented Apr 14, 2023

Copy link
Copy Markdown
Collaborator Author

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 :)

Iron-E added 4 commits April 28, 2023 19:41
If you do:

```lua
require'barbar'.setup {
  icons = {
    diagnostics = {
      [vim.diagnostic.ERROR] = {enabled = true},
      [vim.diagnostic.HINT] = {enabled = true},
    },
  },
}
```

…it will not work. This is because a function in `Buffer` was using
`ipairs`, which stops upon reaching the first `nil` numerical index in a
`table`. However, upon fixing this, I discovered another bug: `icons`
was not being fully merged.

We had a snippet of code to "deep extend" `icons.diagnostics`, but since
we were only using `vim.tbl_deep_extend` on the other `icons` options,
they did not inherit the diagnostics. I extracted this snippet to a
function and called it on all `icons.modified`, `icons.current`, etc.
We reference distinct sides of the barbar often enough that it probably
deserves its own alias.
We already had a `current_buffer_index` variable in
`get_bufferline_containers`, so we can use that instead of storing all
the activities of each buffer in their containers and looping over the
buffers in each container looking for the current buffer,

Apologies for the style changes; I was debugging something else,
and had a really hard time reading this particular section. While
reading through to understand the changes I discovered this performance
improvement.
Removes unecessary `deepcopy`
@Iron-E Iron-E force-pushed the ref/module-naming branch from b3d3efc to c3431aa Compare April 28, 2023 23:41
@Iron-E Iron-E merged commit 488bbaa into master Apr 28, 2023
@Iron-E Iron-E deleted the ref/module-naming branch April 28, 2023 23:42
@Iron-E Iron-E restored the ref/module-naming branch April 29, 2023 21:46
@Iron-E Iron-E deleted the ref/module-naming branch April 29, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Misc. change in an existing component of barbar.nvim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants