lsif-util: Fix #70 by replacing \\" with \" in vertex extra info sections#112
lsif-util: Fix #70 by replacing \\" with \" in vertex extra info sections#112dbaeumer merged 2 commits intomicrosoft:masterfrom
Conversation
…a info sections
|
@jumattos can you please have a look. |
|
@dbaeumer Sure! On it. |
| @@ -75,7 +75,8 @@ function printDOT(edges: { [id: string]: LSIF.Element }, vertices: { [id: string | |||
| Object.keys(extraInfo).forEach((property: string) => { | |||
| const value: string = JSON.stringify((extraInfo as any)[property]); | |||
| const re: RegExp = new RegExp('"', 'g'); | |||
There was a problem hiding this comment.
I made a mistake by not considering already escaped quotes. Thanks for catching that!
The original regex is where the error is. I'd suggest fixing it instead of adding a new one at line 79. You could add a negative lookbehind to avoid matching \", like so:
| const re: RegExp = new RegExp('"', 'g'); | |
| const re: RegExp = new RegExp(/(?<!\\)"/g); |
There was a problem hiding this comment.
Was considering a negative lookahead but was concerned about the performance implications of it. If you think its ok to look past that (or if its a non issue) and youd prefer the neg-lookahead, then id be happy to change it :)
There was a problem hiding this comment.
The neg-lookahead looks cleaner, but that's a valid concern. I added this comment as a suggestion, but I already approved your PR.
If you allow me one nit picking comment, it would be nice if both regex (lines 77 and 79) had the same format. Either they both use strings or they both are between forward slashes. I don't mind either way, but they would look nicer if consistent.
There was a problem hiding this comment.
For sure! Its 00:52 here so I'll get to that tomorrow unless you want to fix that up yourself before then
There was a problem hiding this comment.
The beauty of pull requests is the fact they are asynchronous. No hurry. Have a good night!
There was a problem hiding this comment.
Changed that now. Lot of \ needed with new RegExp 🤔
There was a problem hiding this comment.
It's a matter of preference, and it's your PR. If you want to change both to forward slashes, that's fine. You can also complete it as is.
There was a problem hiding this comment.
I think its a low enough traffic file that we can leave it as is in the mean time. If youre happy with it, merge away :)
There was a problem hiding this comment.
Absolutely. I never investigated further #70 because I planned on rewriting this function to have tailored output depending on the type of vertex (something I've seen before on LSIF visualizers, and it's quite sweet). I wonder how many people are using this tool, and whether this work would be valuable/a priority. Unfortunately I don't have data to back me up on it.
|
@dbaeumer Build is failing here because of mocha. This project doesn't have unit tests. Do you know what's happening? |
4 participants
Cases where extra text would already contain an escaped double quote would result in the \ being escaped, causing the double quote to interfere with graphdot
Closes #70
Not tested on a variety of inputs yet, if more validation is preferred ye can hold off on merging this