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

Avoid matching lowercase files when fname_case feature is missing #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dbarnett
Copy link
Contributor

Put the BUILD, .BUILD, and BUILD. patterns behind a has('fname_case')
check so that on case-insensitive systems it doesn't false positive on
lowercase names. "build" is kind of a generic pattern if it's not
case-sensitive. Other patterns in vim's runtime/filetype.vim perform
similar checks.
See https://groups.google.com/d/topic/vim_dev/m0_mv6E63mE/discussion for
context.

Users can configure an explicit autocmd in their vimrc if they want the
case-insensitive match regardless.

Put the BUILD, *.BUILD, and BUILD.* patterns behind a has('fname_case')
check so that on case-insensitive systems it doesn't false positive on
lowercase names. "build" is kind of a generic pattern if it's not
case-sensitive. Other patterns in vim's runtime/filetype.vim perform
similar checks.
See https://groups.google.com/d/topic/vim_dev/m0_mv6E63mE/discussion for
context.

Users can configure an explicit autocmd in their vimrc if they want the
case-insensitive match regardless.
autocmd BufRead,BufNewFile *.bzl,BUILD,*.BUILD,BUILD.*,WORKSPACE setfiletype bzl
autocmd BufRead,BufNewFile *.bzl,WORKSPACE setfiletype bzl
if has('fname_case')
autocmd BufRead,BufNewFile BUILD,*.BUILD,BUILD.* setfiletype bzl

Choose a reason for hiding this comment

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

Does this mean that this will no longer match BUILD files on OSX?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, and likewise not on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OSX doesn't have fname_case? I didn't see it mentioned in vim's help: "for Amiga, MS-DOS, and Windows this is not present".

Copy link
Contributor

Choose a reason for hiding this comment

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

The poster on the mailing list says they're using OS X.

Also Vim sets CASE_INSENSITIVE_FILENAME on OS X: https://github.com/vim/vim/blob/v8.0.0563/src/os_mac.h#L91

Copy link
Contributor

@malcolmr malcolmr left a comment

Choose a reason for hiding this comment

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

LGTM.

Note that these have already been added as part of vim/vim@6856393.

@artasparks
Copy link

artasparks commented Apr 14, 2017

I would really rather we don't do this. At least, leave BUILD. That's the standard file name for BAZEL, even if it conflicts. If you want to guard *.BUILD or BUILD.*, ok (but I think this significantly neuters the usefulness of these).

Also, the problem here reported is that there is a shell script called build. Should this be caught by a hash-bang declaration?

@artasparks
Copy link

Note that since Open Source was changed (which again, I'm -1 on in its current form for the reasons above), it makes even more sense to leave this as is. If you include this plugin, you're intentionally including it, which means that you to have BUILD files detected correctly.

@malcolmr
Copy link
Contributor

I was thinking in terms of this being the upstream for Vim, but you make a good point about people including this plugin explicitly.

@dbarnett
Copy link
Contributor Author

I agree it'd be a shame and didn't realize it affected OSX, too. I'll wait and see what happens on vim/vim#1340.

Re keeping the case-insensitive behavior in this plugin, in general I'd prefer to not have standing diffs here that aren't merged into vim. If there's no workable solution for the built-in version, I guess it'd have to do, though.

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.

4 participants