Adding state to substitution command #3068
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,40 +128,67 @@ function parseCount(scanner: Scanner): number { | |
| */ | ||
| export function parseSubstituteCommandArgs(args: string): node.SubstituteCommand { | ||
| try { | ||
| let searchPattern: string | undefined; | ||
| let replaceString: string; | ||
| let flags: number; | ||
| let count: number; | ||
|
|
||
| if (!args) { | ||
| // special case for :s | ||
| return new node.SubstituteCommand({ | ||
| pattern: undefined, | ||
| replace: '', // ignored in this context | ||
|
There was a problem hiding this comment. I'm thinking if we can use the same rule for There was a problem hiding this comment. Ah. I remember wanting to do this, but totally forgot that there was a reason why I couldn't. The replacement actually works differently than the pattern in vim. Contrary to pattern, "empty replacement" is always interpreted by vim as "a replacement with nothing", no matter what the previous replacement was ;) (EXCEPT for the ":s" case, which works differently). So, for example, if you do a ":s/this/that/", and then do a ":s/" or a ":s//", ":s///g", etc... it will always delete occurrences of "this". I'm glad you brought this up, because it seems I missed this case in the tests :-! I'll definitely add it :).
There was a problem hiding this comment. Hi @rebornix I added a new test to check for all the variations of empty strings. Please let me know if reading the new test resolves this item. With that in mind, I do understand that this totally isn't the easiest code to read. Now that all the different sequences of events are pretty well covered by tests, if you have any other ideas you'd like me to try in line with this, I'll be happy to try them, and we can be sure it'll have the same behavior :). |
||
| flags: node.SubstituteFlags.None, | ||
| count: 1, | ||
| }); | ||
| } | ||
| let scanner: Scanner; | ||
|
|
||
| let delimiter = args[0]; | ||
|
|
||
| if (isValidDelimiter(delimiter)) { | ||
| if (args.length === 1) { | ||
| // special case for :s/ or other delimiters | ||
| return new node.SubstituteCommand({ | ||
| pattern: '', | ||
| replace: '', | ||
| flags: node.SubstituteFlags.None, | ||
| count: 1, | ||
| }); | ||
| } | ||
|
|
||
| let secondDelimiterFound: boolean; | ||
|
|
||
| scanner = new Scanner(args.substr(1, args.length - 1)); | ||
| [searchPattern, secondDelimiterFound] = parsePattern('', scanner, delimiter); | ||
|
|
||
| if (!secondDelimiterFound) { | ||
| // special case for :s/search | ||
| return new node.SubstituteCommand({ | ||
| pattern: searchPattern, | ||
| replace: '', | ||
| flags: node.SubstituteFlags.None, | ||
| count: 1, | ||
| }); | ||
| } | ||
| replaceString = parsePattern('', scanner, delimiter)[0]; | ||
| } else { | ||
| // if it's not a valid delimiter, it must be flags, so start parsing from here | ||
| searchPattern = undefined; | ||
| replaceString = ''; | ||
| scanner = new Scanner(args); | ||
| } | ||
|
|
||
| scanner.skipWhiteSpace(); | ||
| flags = parseSubstituteFlags(scanner); | ||
| scanner.skipWhiteSpace(); | ||
| count = parseCount(scanner); | ||
|
|
||
| return new node.SubstituteCommand({ | ||
| pattern: searchPattern, | ||
| replace: replaceString, | ||
| flags: flags, | ||
| count: count, | ||
| }); | ||
| } catch (e) { | ||
| throw error.VimError.fromCode(error.ErrorCode.E486); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import { Position } from './../common/motion/position'; | ||
| import { TextEditor } from './../textEditor'; | ||
|
|
||
| /** | ||
| * State involved with Substitution commands (:s). | ||
| */ | ||
| export class SubstituteState { | ||
| /** | ||
| * The last pattern searched for in the substitution | ||
| */ | ||
| public searchPattern: string; | ||
|
|
||
| /** | ||
| * The last replacement string in the substitution | ||
| */ | ||
| public replaceString: string; | ||
|
|
||
| constructor(searchPattern: string, replaceString: string) { | ||
| this.searchPattern = searchPattern; | ||
| this.replaceString = replaceString; | ||
| } | ||
| } |
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.
Could this part be simplified to
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.
If I'm understanding you correctly, I think this is also related to the response below. I tried to make comments to make it clearer, but clearly I should redo them. What do you think if instead of scattering comments into the cases, I write a big comment at the top here, as well as at the top of the substitute parser?
Do you think a big verbose comment like this would be helpful? Or do you think the tests would be documentation enough?
By the way, one of the reasons I tried so hard to mimic all the cases of cross-state behavior is that I subconsciously rely on this behavior myself with little habits my mind has (e.g. ":s/this/that" then "n" to find the next one, then decide if I want to ":s" or not, then "n"...)
I remember when I first tried to use another IDE-vim thing (I think it was visvim), my habits weren't getting the results I subconsciously expected, so I dropped it - I didn't quite get why it wasn't right until I worked on this issue :P. What I've been loving about VSCodeVim is that whatever's been implemented works freaking perfectly. It really feels like best-of-both-worlds instead of worst-of-both-worlds ;).
PS - I mean no disrespect to visvim - maybe visvim works great now - it must have been at least ten years ago that I'm talking about.
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.
@rebornix I decided to add that big verbose comment after all. Feel free to let me know if it's helpful or not.
And again, since tests for all the different sequences are pretty well covered, if you have any suggestions to make this implementation more readable without the comments, I'm all ears :).