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

Substitute global flag (like Vim's gdefault) #1909

Merged
merged 7 commits into from
Jul 10, 2017

Conversation

philipmat
Copy link
Contributor

@philipmat philipmat commented Jul 8, 2017

"vim.substituteGlobalFlag"

  • Similar to Vim's gdefault setting.
  • /g flag in a substitute command replaces all occurrences in the line.
    Without this argument, replacement occurs only for the first occurrence in each line.
  • When "vim.substituteGlobalFlag" is true, the 'g' is default on.
    This means that all matches in a line are substituted instead of one.
    When a 'g' flag is given to a ":substitute" command, this will toggle the substitution
    of all or one match.

@Chillee
Copy link
Member

Chillee commented Jul 8, 2017

Seems like there are relevant tests failing?

If the tests are too annoying to work properly, I am mostly fine (ehh) with removing the failing tests. The logic is very simple and I'm almost certain it'll work.

Also, I just realized know why the tests are failing. The tests are set up to use neovim integration for the Ex-commands. That's another thing to consider about this PR, it'll only affect non-neovim integration users.

@philipmat
Copy link
Contributor Author

philipmat commented Jul 8, 2017

@Chillee - the tests that are failing are actually the very tests that I've added for this feature. To my fault, I did test with the neovim flag set to false.

Let me see install neovim and see if I can make it work with true.

@Chillee
Copy link
Member

Chillee commented Jul 8, 2017

@philipmat Do the tests run fine locally?

To be clear:

  1. The code you've added only affects :%s for non-neovim Ex mode.
  2. The current tests for Ex-mode are run for neovim Ex mode.
  3. If you are having difficulty getting the tests to work properly, I am fine with having no tests for this configuration option specifically. The logic is simple and I'm almost certain there will be no bugs related to it.

@philipmat
Copy link
Contributor Author

The tests work for enableNeovim=false because the /g flag is being set in substitute.ts according to the proposed substituteGlobalFlag`.

I need to find a way to pass this configuration flag to Neovim so that tests with enableNeovim=true work just as well.

If you see more value in having this flag in right now, I'd be happy to take the offending tests out; however, if it's all the same to you I'd rather have the tests and make them work right.

Although, it just occurred to me that due to testUtils.ts enabling neovim for all tests, that effectively bypasses testing the SubstituteCommand from substitute.ts.

@Chillee
Copy link
Member

Chillee commented Jul 8, 2017

Yeah the testutils part is what I was talking about. Ex mode without neovim integration is in somewhat of a "maintenance" mode, where it's unlikely that we'll add features to it that can be covered by neovim.

@Chillee
Copy link
Member

Chillee commented Jul 9, 2017

Looks good to me! Could you add a note to the configuration that it only works when not using the neovim integration for Ex-commands?

Thanks!

@philipmat
Copy link
Contributor Author

@Chillee - with b70942e it actually works with neovim too - which is why it passes the tests.

@Chillee Chillee merged commit 805b03d into VSCodeVim:master Jul 10, 2017
@Chillee
Copy link
Member

Chillee commented Jul 10, 2017

Ah nice. Looks good to me!

This pull request was closed.
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