Skip to content

Conversation

@TamaMcGlinn
Copy link
Contributor

@TamaMcGlinn TamaMcGlinn commented May 20, 2022

In your vimrc you can do e.g.

call glaive#Install()
Glaive codefmt buildifier_lint_mode='fix'

to get buildifier to autofix issues. The default behaviour is unchanged.

TamaMcGlinn added a commit to TamaMcGlinn/vimrc that referenced this pull request May 20, 2022
this requires vim-codefmt PR 192 merged
google/vim-codefmt#192
@dbarnett
Copy link
Contributor

Wouldn't it be better to by default not pass any mode override at all? This would be another layer of defaulting that could fight with other local config settings, etc.

@TamaMcGlinn
Copy link
Contributor Author

I'm passing the default value though, so that will do exactly the same. Anyway, I like being explicit about the lint_mode. If this could fight with other local config settings, that means you're saying the lint_mode can already be specified in some other way, and the whole PR would be unnecessary. As far as I know, that's not the case, but if so, please elaborate on how to do that.

@dbarnett
Copy link
Contributor

I don't know whether buildifier would have any other override mechanism in practice or not, just that I prefer not to copy-paste defaults. IOW, you're saying it will do the same because you're passing the same default value… what if they change the default later, like add a new middle option for minimal fixes and make that the default? Would we want this default to take precedence over theirs?

@TamaMcGlinn
Copy link
Contributor Author

okay fair enough. so it should be changed to have the empty string as the default, and an if-statement added to not add the parameter in that case.

@TamaMcGlinn
Copy link
Contributor Author

Okay, fixed that. Also tested and specified in the help message what the behaviour is for 'warn', since it is possible someone would actually choose that behaviour.

let l:lint_flag = s:plugin.Flag('buildifier_lint_mode')
let l:cmd = [ s:plugin.Flag('buildifier_executable') ]
if l:lint_flag != ""
let l:cmd += ["--lint=" .. l:lint_flag]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this .. a typo intended to be .? AFAICT it blows up with an error on my version of vim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How strange! I'm using NeoVim and always used two dots to concatenate strings, I never knew one dot sufficed. It's fixed now.

@dbarnett
Copy link
Contributor

Thanks for working with me on the defaults! Looks good now besides one .. thing that might be broken I commented on.

In your vimrc you can do e.g.

call glaive#Install()
Glaive codefmt buildifier_lint_mode='fix'

to get buildifier to autofix issues. The default behaviour is unchanged.
@dbarnett dbarnett merged commit f393d51 into google:master May 30, 2022
@dbarnett
Copy link
Contributor

Merged, thanks!

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