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

Don't clobber global grepprg, or quickfix mappings #90

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

Conversation

ches
Copy link

@ches ches commented Feb 7, 2015

'grepprg' is a buffer-local setting. We should only use the &l version, or else when we restore it after an :Ag command, we're disrupting the setting in other buffers.

It's surprising that 'grepprg' is local and global, and 'grepformat' is only global, so I guess we have no choice but to clobber it. Perhaps this has it's uses: suppose you have a C program with some tests in shell (or cram, etc., like ag!), and you always use ag for 'grepprg'. The same 'grepformat' can work project-wide, but you might have an autocommand that makes 'grepprg' use ag --shell for all files under the tests directory.

Anyway, it's kind of pathological, but we just shouldn't clobber a global setting when local can be used. I was working on something else when I made the small changes here, but that's going to warrant deeper review discussion when it's ready I think, so I went ahead and isolated these changes.

Update: I realized the <buffer> mappings were broken too, <buffer> must be given first or it doesn't work. Fix included.

'grepprg' is a buffer-local setting. We should only use the `&l`
version, or else when we restore it after an `:Ag` command, we're
disrupting the setting in other buffers.

It's surprising that 'grepprg' is local and global, and 'grepformat' is
only global, so we have no choice but to clobber it. But this could have
it's uses: suppose you have a C program with some tests in shell (like
ag!), and you always use `ag` for 'grepprg'. The same 'grepformat' can
work project-wide, but you might have an autocommand that makes
'grepprg' use `ag --shell` for all files under the tests directory.
`<buffer>` *must* come first. This was leaving the mappings set in all
normal quickfix windows, which is not our place to impose on users.
@ches ches changed the title Don't clobber global grepprg Don't clobber global grepprg, or quickfix mappings Feb 7, 2015
@ches
Copy link
Author

ches commented Feb 13, 2015

The last commit here was a little cleanup of a bug I left earlier. I'll squash the commits if this otherwise meets review approval.

Is there anything controversial here?

@losingkeys
Copy link
Collaborator

Wow, nice work. I have lots of questions (sorry in advance lol).

  • I understand not clobbering the quickfix mappings, but aren't we resetting the grepprg after using it? Don't we want to do that so other plugins can have buffer-local mappings and we won't break those except when :Ag is running (short amounts of time)?

Some style comments/preferences (these are up for debate, but I'd prefer at least consistency if we choose something else):

  • The plugins in $VIMRUNTIME/plugins/ all use g:loaded_<plugin-name> for their load once section, and I think I've seen this elsewhere. Mind renaming g:autoloaded_ag?
  • As far as aligning = goes, it really only matters to me that things are consistent (you'll notice that they're not even before this pull, but maybe this is a good time to fix them). So a=b, a = b, or aligned with other =s in the same small block are all fine, but let's make them all match. Also formatting should be separate commits imo.

Other thoughts that might not pertain:

  • It's odd that it just ignores <buffer>, maybe that's a bug in vim (though the docs mention it, I'm not sure why this limitation exists)
  • Prefixing ex commands w/: doesn't really matter to me, as long as it works. So I'd say as long as things are consistent it's good. Mind checking if all our ex commands follow the same style?

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.

None yet

2 participants