-
Notifications
You must be signed in to change notification settings - Fork 104
Add a google-java formatter. #78
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
Conversation
Fixes: google#51 This commit adds a google-java-formatter. Some notes: - Mostly a port from the internal version. - I fixed a bug along the way, where if you didn't have clangformat installed, codefmt would error-out during tab-complete. - I fixed a typo in buildifier. - And some minor updates to the README
README.md
Outdated
| augroup END | ||
| ``` | ||
|
|
||
| # Configuring formatters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop trailing period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
| start typing flag names and use tab completion. See `:help Glaive` for usage | ||
| details. | ||
|
|
||
| # Installing and configuring formatters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this section name since you already have "Configuring formatters" above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
README.md
Outdated
| * Create a flag in instant/flags.vim | ||
| * Create a flag in | ||
| [instant/flags.vim](https://github.com/Kashomon/vim-codefmt/blob/master/instant/flags.vim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant [instant/flags.vim](instant/flags.vim) rather than linking to your fork here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to google. Does the short link thing work? I've never tried that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it does. Just verified using the Preview feature in GitHub's web editor. It's worth using that style because (a) it will link to the appropriate branch/fork/commit wherever you use it, and (b) it fits on one line that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
autoload/codefmt/clangformat.vim
Outdated
| function! s:ClangFormatHasAtLeastVersion(minimum_version) abort | ||
| if !exists('s:clang_format_version') | ||
| let l:executable = s:plugin.Flag('clang_format_executable') | ||
| if !executable(l:executable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to pull this out into a separate PR since it looks independent and tests are failing on it as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy enough to fix with an override. I also had to fix the clangformat vroom tests because google-java-format takes precedence, I guess.
autoload/codefmt/googlejava.vim
Outdated
| \ 'setup_instructions': 'Install google-java formatter ' . | ||
| \ "(https://github.com/google/google-java-format). \n" . | ||
| \ 'Enable with "Glaive codefmt google_java_executable=' . | ||
| \ '"java -jar /path/to/google-java-format-1.2-all-deps.jar" ' . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use placeholder version numbers like /path/to/google-java-format-X.Y-all-deps.jar so we don't get pull reqs bumping this later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
autoload/codefmt/googlejava.vim
Outdated
| call maktaba#ensure#IsNumber(l:startline) | ||
| call maktaba#ensure#IsNumber(l:endline) | ||
| endfor | ||
| let l:ranges_str = join(map(a:ranges, 'v:val[0] . ":" . v:val[1]'), ',') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use copy() to avoid surprises w/ modifying a:ranges since vim's map operates in-place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean copy(a:ranges)? Done
autoload/codefmt/googlejava.vim
Outdated
| call maktaba#buffer#Overwrite(1, line('$'), l:formatted) | ||
| endfunction | ||
|
|
||
| let s:google_java_format = l:formatter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s:google_java_format looks unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
clang-format is, apparently, the default java formatter. ok
dbarnett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
README.md
Outdated
| call glaive#Install() | ||
| " Optional: Enable codefmt's default mappings on the <Leader>= prefix. | ||
| Glaive codefmt plugin[mappings] | ||
| Glaive codefmt google_java_executable="java -jar /path/to/google-java-format-1.2-all-deps.jar" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/1.2/VERSION/ here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Fixes: #51
This commit adds a google-java-formatter. Some notes:
installed, codefmt would error-out during tab-complete.