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

Neovim and Tmux improvements #34

Merged
merged 10 commits into from
Aug 8, 2021
Merged

Neovim and Tmux improvements #34

merged 10 commits into from
Aug 8, 2021

Conversation

nandalopes
Copy link

  • style(nvim): group together colorschemes
  • feat(nvim): allow local plugins
  • fix(nvim): powerline extended symbols
  • feat(nvim): OS specific keymaps
  • style(nvim): align comments on plugin set
  • refactor(tmux): many improvements

Neovim and LightLine colorschemes in the same file
- Highlight window name with purple bold when zoomed
- Renumber windows
- Separate colorscheme from other styling settings
- Use `none` instead of `nobold,noitalic,...`
- Restore clear screen keybind mapped over by tmux-navigator
Copy link
Owner

@lfilho lfilho left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for the contribution!! 💪

Apart the questions/comments I posted on the diff view, it looks likes there's a some new things, other things being changed, but you haven't documented / explained them here in the PR description. That alongside the several changes in indentation, spaces and styles, makes it really hard to review the PR.

I'd personally recommend not to change styles like that unless we can make it as part of automatic linting/tooling, otherwise it becomes a matter of personal taste (then next person sending a PR will change it to something THEY like, so on so on...).

If you insist on changing the style, at least comment on each diff what is actually changing / new versus just style changes.

@@ -1,37 +1,36 @@

Copy link
Owner

@lfilho lfilho Jul 14, 2021

Choose a reason for hiding this comment

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

The diff is hard to parse now. Is this just changing some of the order the comments' indentation?

Copy link
Owner

Choose a reason for hiding this comment

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

Unless there's a plugin automatically indenting the comments like this, I would prefer to leave as they were before otherwise it would be tiresome to keep maintaining and changing spacing around them. So could you please either provide that auto-formatting support or keep the previous commenting style? Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Yes there is Tabular, in improvements bundle

Copy link
Owner

Choose a reason for hiding this comment

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

But is not automatically formatting the file, right?

Copy link
Author

Choose a reason for hiding this comment

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

No, it's not like an autocmd, but can be easily triggered using the plugin included (Tabularize)

Copy link
Owner

Choose a reason for hiding this comment

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

Exactly, that's my point: if we don't have an automatic way to guarantee formatting, better not to change it. Otherwise here's what happens:

  1. There's one personal cosmetic preference currently (mine)
  2. You send a big diff with your own personal preference (this PR)
  3. From now on me and future contributors have to adjust and remember to use Tabularize in those files (extra annoying manual work)
  4. A future contributor don't like your personal preference from step 2 and sends a big PR to either revert it back to the way it was on step 1 or worse, proposes a 3rd way 😓
  5. The cycle repeats, including this very conversation 😄

It's 2021, styling/formatting shouldn't be done manually. If there's not a way to make the auto-formatting work or if it's not worth the effort, then the recommendation when contributing to an existing project is to leave as it is when you found and follow the repo's current style in order to avoid unnecessary diffs and discussions around personal preferences. Makes sense?

@@ -19,4 +19,10 @@ for fpath in split(globpath(pluginPath, '*.vim'), '\n')
endif
endfor

" The plugins listed in ~/.config/nvim/.local.vim will be added here to
Copy link
Owner

Choose a reason for hiding this comment

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

We already support this. Check the the before/main and after/main files. The comments in there explain it. nvim/README also documents and explains this...

Copy link
Author

Choose a reason for hiding this comment

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

It's intended to local plugin aditions, not after and before settings overrides.

Copy link
Owner

Choose a reason for hiding this comment

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

But we actually can install custom plugins in the current after files, right? So my question is why do we need a second way of accomplishing that..?

Copy link
Author

Choose a reason for hiding this comment

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

No we don't, unless we add another .vim in plugins folder, and that file will be tracked anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

What I mean is that we can add a Plug install <bla> in the after file and it would work to install plugins, wouldn't it?
Also, it's best to let the author of a comment resolve that thread instead of resolving it ourselves 😇

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm have you actually tested it? I have just tested it and it works:

  1. created ~/.yadr/nvim/settings/after/test.vim
  2. added Plug 'pineapplegiant/spaceduck', { 'branch': 'main' } in it (it's a colorscheme)
  3. ran :PlugInstall
  4. ran :colors spaceduck
  5. Worked, without the test.vim being tracked

Copy link
Owner

@lfilho lfilho Jul 22, 2021

Choose a reason for hiding this comment

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

Maybe you missed this?

dotfiles/.gitignore

Lines 18 to 21 in 50db8ba

nvim/settings/before/*.vim
!nvim/settings/before/main.vim
nvim/settings/after/*.vim
!nvim/settings/after/main.vim

It trackes the */main.vim in those folders, without tracking any other *.vims in them...

Copy link
Author

Choose a reason for hiding this comment

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

It installs the plugin, but it doesn't get loaded after restarting nvim:

Erro detectado ao processar /home/nanda/.dotfiles/neovim/settings/after/colorscheme.vim:
linha   10:
E185: Esquema de cores 'spaceduck' não encontrado

Copy link
Author

Choose a reason for hiding this comment

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

Adendum: it's not necessary to explicit load .local.vim on plugins/main.vim, but is mandatory to load Plug there, otherwise you won't get the plugin after restarting nvim.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, that's correct. That's the default behavior. Upon installing we do manually trigger Plug so it can install the plugins, though. Anything post-install needs to have an explicit call to PlugInstall.

But I'm confused why you brought that up, that would also happen with your proposed changes, right?

Copy link
Author

@nandalopes nandalopes left a comment

Choose a reason for hiding this comment

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

I'm sorry for this mono-PR. I comment on commits the changes.

@@ -19,4 +19,10 @@ for fpath in split(globpath(pluginPath, '*.vim'), '\n')
endif
endfor

" The plugins listed in ~/.config/nvim/.local.vim will be added here to
Copy link
Author

Choose a reason for hiding this comment

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

It's intended to local plugin aditions, not after and before settings overrides.

@nandalopes nandalopes requested a review from lfilho July 15, 2021 00:41
@nandalopes
Copy link
Author

Tmux improvements:

  • Highlight window name with purple bold font when zoomed
  • Renumber windows
  • Separate colorscheme from other styling settings
  • Use none style attribute instead of nobold,noitalic,...
  • Restore clear screen keybind mapped over by tmux-navigator
    • C-l changes pane
    • Prefix + C-l clear screen

Neovim improvements:

  • Use nerdfonts powerline glyphs
  • Source local plugins commands at plugins/main.vim, before plug#end()

Put your {name-of-local-bundle}.vim on plugins folder, your plugin will be
loaded across program restarts
@@ -19,3 +19,5 @@ nvim/settings/before/*.vim
!nvim/settings/before/main.vim
nvim/settings/after/*.vim
!nvim/settings/after/main.vim
nvim/plugins/*.vim
Copy link
Owner

Choose a reason for hiding this comment

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

This can't be here: it will make all current files in there not be tracked by git anymore. So if we change any configuration, or add a new plugin and configuration file later on, it wouldn't be picked up by git. Only the before and after files should be here, because only those ones are supposed to be untracked by git (things users can add post install to their liking)

I think this is the only remaining thing in the PR, otherwise it's looking good! Thanks!!

Copy link
Author

Choose a reason for hiding this comment

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

Actually git remains aware of changes in files that are already added.
Any change in the files on repo will be noticed, only tools that use .gitignore patterns to exclude search will be affected, like ripgrep without --no-vcs-ignore opt.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, but what if we want to add a new plugin (e.g.: right now i have a coc branch and in the future i might merge it in, in that case the new files in there wouldn't get tracked and my changes would stay local to my machine)?

What benefits did you have in mind by adding this here? I am not seeing them yet, only the issue i highlight above :)

Copy link
Author

Choose a reason for hiding this comment

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

For example, at work I manage some PHP files, but I don't want to add a PHP bundle on personal computer.

At home I use vimwiki, but not at work., and so on.

I have a work branch, which I've forced add a web.vim bundle, with git add -f <file>, and a local_bundles.vim file at home, not tracked, with vimwiki.

Copy link
Owner

Choose a reason for hiding this comment

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

Got it. I still don't see a big reason to do it, but I guess it doesn't hurt to try. Will merge. Thanks again for the nice contributions! I'm about to push some more as well

@lfilho lfilho merged commit 744bfe3 into lfilho:master Aug 8, 2021
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.

2 participants