| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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.
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.
Cool stuff @xiemaisi, LGTM but that last one is bit tricky so someone else should check it too.
src/extensibility/Package.js
Outdated
| @@ -265,7 +265,7 @@ define(function (require, exports, module) { | |||
| // Download the bits (using Node since brackets-shell doesn't support binary file IO) | |||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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.
LGTM with a comment, someone else should do a double check too. @saurabh95 could you do that 🙏
| @@ -145,7 +145,7 @@ define(function (require, exports, module) { | |||
| t.kind = kind; | |||
| t.origin = "ecmascript"; | |||
| if (kind === "string") { | |||
| if (/[\\\\]*[^\\]"/.test(t.value)) { | |||
| if (/[^\\]"/.test(t.value)) { | |||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one looks scary to me as well. Will do some testing around it.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok. seems good to me. We can merge this after 1.10 release
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.
Good stuff @xiemaisi, merging this in and will land in 1.11. Hopefully we get the other fixes found by lgtm in too.
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.