Conversation
The previous regexp looked a little dubious, since `[\\\\]` means exactly the same as `[\\]`. Note, however, that `/x*y/.test(s)` returns true iff `/y/.test(s)` does, so the character class was redundant to begin with.
| var r = extensionManager.downloadFile(downloadId, urlInfo.url, PreferencesManager.get("proxy")); | ||
| r.done(function (result) { | ||
| d.resolve({ localPath: FileUtils.convertWindowsPathToUnixPath(result), filenameHint: urlInfo.filenameHint }); | ||
| d.resolve({ localPath: FileUtils.convertWindowsPathToUnixPath(result), filenameHint: filename }); |
There was a problem hiding this comment.
This makes sense but I am not 100% sure either, @swmitra?
xiemaisi
force-pushed
the
fix-lgtm-alerts
branch
from
abf3c77 to
3bfa3d0
Compare
|
I've removed the last two commits with the less obvious fixes; I'll put them up as a separate PR. |
|
@petetnt, does this look safer now? |
petetnt
left a comment
petetnt
left a comment
There was a problem hiding this comment.
LGTM with a comment, someone else should do a double check too. @saurabh95 could you do that 🙏
| t.origin = "ecmascript"; | ||
| if (kind === "string") { | ||
| if (/[\\\\]*[^\\]"/.test(t.value)) { | ||
| if (/[^\\]"/.test(t.value)) { |
There was a problem hiding this comment.
This looks bit scary to me, but didn't manage to find any differences after running some tests cases by hand 🤔
There was a problem hiding this comment.
This one looks scary to me as well. Will do some testing around it.
There was a problem hiding this comment.
It's not as scary as you might think: the original code looks for a substring of t.value that contains zero or more backslashes, followed by something that isn't a backslash, followed by a double quote. This is the same as simply looking for a substring consisting of something that isn't a backslash followed by a double quote, which is what the new code does.
(Note that lgtm actually only highlighted the fact that [\\\\] is redundant and means the same as [\\] or indeed \\.)
There was a problem hiding this comment.
Oh ok. seems good to me. We can merge this after 1.10 release
There was a problem hiding this comment.
Good stuff @xiemaisi, merging this in and will land in 1.11. Hopefully we get the other fixes found by lgtm in too.
4 participants
This fixes a few minor issues found by lgtm; for more details see https://lgtm.com/projects/g/adobe/brackets/alerts/.
The fixes in the first three commits are fairly obvious, the last two commits involve some educated guessing as to the intended semantics of the code involved.