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

Use vim's builtin format mapping #121

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use vim's builtin format mapping #121

wants to merge 2 commits into from

Conversation

tbodt
Copy link
Member

@tbodt tbodt commented Jul 6, 2019

No description provided.

Copy link
Contributor

@dbarnett dbarnett left a comment

Choose a reason for hiding this comment

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

I like providing a simple hook for people who want to override formatexpr and an example of how. It would be pretty aggressive behavior to unconditionally override formatexpr, though. Could be based on a setting, but for things like this it's usually more flexible and less brittle to provide the pieces and let the user hook it up themselves. For instance, if they wanted to override formatexpr for some but not all filetypes, they'd come asking how to adapt this to do what they wanted and we wouldn't have a simple clean way to offer them.

What do you think about changing it so the autocmd is in your vimrc and just calls a simple codefmt helper?

Copy link
Contributor

@dbarnett dbarnett left a comment

Choose a reason for hiding this comment

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

I think it needs to be opt-in. See my previous longer comment in Conversation.

@tbodt
Copy link
Member Author

tbodt commented Mar 2, 2020

Removed autocmd from mappings file, PTAL

Copy link
Contributor

@dbarnett dbarnett left a comment

Choose a reason for hiding this comment

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

LG to merge besides getting the functions into the generated help docs. LMK if you need any help with that part.

Comment on lines +270 to +282
if !empty(get(b:, 'codefmt_formatter'))
let l:Predicate = {f -> f.name ==# b:codefmt_formatter}
else
let l:Predicate = {f -> f.AppliesToBuffer() && s:IsAvailable(f)}
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

For reference: We haven't been using lambdas in any of our plugins yet, but I checked and they were originally added in 7.4.2044 from 2016. That's probably long enough ago. At some point I should probably declare a min supported vim version for the plugin and get it testing against that version, though.

@@ -252,6 +262,23 @@ function! codefmt#GetSupportedFormatters(ArgLead, CmdLine, CursorPos) abort
return join(l:groups[0] + l:groups[1] + l:groups[2], "\n")
endfunction

""
" Returns whether there is a default formatter available for the current
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make this @public and regenerate the vimdoc?

It should have a sample of how to use it somewhere in the help docs, but for that part I'm happy to merge it w/o and add the docs after.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (two years later :)

TruncatedDinoSour pushed a commit to TruncatedDinoSour/vim-codefmt that referenced this pull request Jan 15, 2022
@dbarnett
Copy link
Contributor

Thanks!

Removed autocmd from mappings file, PTAL

Looks like the autocmd is still in the mappings file? I'd suggest to instead restructure a bit to define a codefmt#SetFormatExprIfAvailable and then replace the unconditional autocmd with some instructions in the help file / README to recommend putting these lines in your vimrc:

augroup codefmt_formatexpr
  autocmd!
  autocmd BufEnter * call codefmt#SetFormatExprIfAvailable()
augroup END

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants