Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Fix lgtm alerts #13483

Merged
petetnt merged 3 commits into from
Jul 8, 2017
Merged

Fix lgtm alerts #13483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c565519
Simplify a regular expression.
max-schaefer Jun 26, 2017
c7c2649
Remove a few spurious trailing arguments.
max-schaefer Jun 26, 2017
3bfa3d0
Remove a few unused local variables.
max-schaefer Jun 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/LiveDevelopment/MultiBrowserImpl/language/HTMLSimpleDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ define(function (require, exports, module) {
*/
Builder.prototype.build = function (strict, markCache) {
var self = this;
var token, lastClosedTag, lastTextNode, lastIndex = 0;
var token, lastClosedTag, lastTextNode;
var stack = this.stack;
var attributeName = null;
var nodeMap = {};
Expand Down Expand Up @@ -441,7 +441,6 @@ define(function (require, exports, module) {
newNode.update();
}
}
lastIndex = token.end;
}

// If we have any tags hanging open (e.g. html or body), fail the parse if we're in strict mode,
Expand Down
2 changes: 1 addition & 1 deletion src/command/Menus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,7 @@ define(function (require, exports, module) {
KeyBindingManager.addBinding(commandID, keyBindings);

// Look for existing key bindings
_addExistingKeyBinding(menuItem, commandID);
_addExistingKeyBinding(menuItem);

menuItem._checkedChanged();
menuItem._enabledChanged();
Expand Down
6 changes: 3 additions & 3 deletions src/extensibility/node/ExtensionManagerDomain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ function _cmdInstall(packagePath, destinationDirectory, options, callback, pCall
// If there was trouble at the validation stage, we stop right away.
if (err || validationResult.errors.length > 0) {
validationResult.installationStatus = Statuses.FAILED;
deleteTempAndCallback(err, validationResult);
deleteTempAndCallback(err);
return;
}

Expand Down Expand Up @@ -287,7 +287,7 @@ function _cmdInstall(packagePath, destinationDirectory, options, callback, pCall
// both legacy and new extensions installed.
fs.remove(legacyDirectory, function (err) {
if (err) {
deleteTempAndCallback(err, validationResult);
deleteTempAndCallback(err);
return;
}
_removeAndInstall(packagePath, installDirectory, validationResult, deleteTempAndCallback);
Expand All @@ -298,7 +298,7 @@ function _cmdInstall(packagePath, destinationDirectory, options, callback, pCall
} else if (hasLegacyPackage) {
validationResult.installationStatus = Statuses.NEEDS_UPDATE;
validationResult.name = guessedName;
deleteTempAndCallback(null, validationResult);
deleteTempAndCallback(null);
} else {
_checkExistingInstallation(validationResult, installDirectory, systemInstallDirectory, deleteTempAndCallback);
}
Expand Down
2 changes: 1 addition & 1 deletion src/extensions/default/JavaScriptCodeHints/HintUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Copy Markdown
Collaborator

@petetnt petetnt Jun 29, 2017

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 🤔

Copy link
Copy Markdown
Contributor

@saurabh95 saurabh95 Jun 29, 2017

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.

Copy link
Copy Markdown
Contributor Author

@xiemaisi xiemaisi Jun 30, 2017

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 \\.)

Copy link
Copy Markdown
Contributor

@saurabh95 saurabh95 Jul 4, 2017

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

Copy link
Copy Markdown
Collaborator

@petetnt petetnt Jul 8, 2017

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.

t.delimiter = SINGLE_QUOTE;
} else {
t.delimiter = DOUBLE_QUOTE;
Expand Down
6 changes: 1 addition & 5 deletions src/extensions/default/JavaScriptCodeHints/Session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,13 +464,9 @@ define(function (require, exports, module) {
var propertyLookup = false,
context = null,
cursor = this.getCursor(),
token = this.getToken(cursor),
lexical;
token = this.getToken(cursor);

if (token) {
// if this token is part of a function call, then the tokens lexical info
// will be annotated with "call"
lexical = getLexicalState(token);
if (token.type === "property") {
propertyLookup = true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/language/CSSUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ define(function (require, exports, module) {
}

if (_isInPropName(ctx)) {
return _getPropNameInfo(ctx, ctx.editor);
return _getPropNameInfo(ctx);
}

if (_isInPropValue(ctx)) {
Expand Down
3 changes: 1 addition & 2 deletions src/language/HTMLSimpleDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ define(function (require, exports, module) {
*/
Builder.prototype.build = function (strict, markCache) {
var self = this;
var token, lastClosedTag, lastTextNode, lastIndex = 0;
var token, lastClosedTag, lastTextNode;
var stack = this.stack;
var attributeName = null;
var nodeMap = {};
Expand Down Expand Up @@ -479,7 +479,6 @@ define(function (require, exports, module) {
newNode.update();
}
}
lastIndex = token.end;
}

// If we have any tags hanging open, fail the parse if we're in strict mode,
Expand Down
2 changes: 0 additions & 2 deletions src/project/WorkingSetView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,6 @@ define(function (require, exports, module) {
gTop,
gHeight,
gBottom,
deltaY,
containerOffset,
scrollerTopArea,
scrollerBottomArea;
Expand Down Expand Up @@ -409,7 +408,6 @@ define(function (require, exports, module) {
gTop = $ghost.offset().top;
gHeight = $ghost.height();
gBottom = gTop + gHeight;
deltaY = pageY - e.pageY;

// data to help us determine if we have a scroller
hasScroller = $item.length && $container.length && $container[0].scrollHeight > $container[0].clientHeight;
Expand Down
3 changes: 1 addition & 2 deletions src/widgets/bootstrap-scrollspy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,11 @@

, refresh: function () {
var self = this
, $targets

this.offsets = $([])
this.targets = $([])

$targets = this.$body
this.$body
.find(this.selector)
.map(function () {
var $el = $(this)
Expand Down
2 changes: 0 additions & 2 deletions src/widgets/bootstrap-tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@
var $tip
, pos
, actualWidth
, actualHeight
, placement
, tp
, e = $.Event('show')
Expand Down Expand Up @@ -143,7 +142,6 @@
pos = this.getPosition()

actualWidth = $tip[0].offsetWidth
actualHeight = $tip[0].offsetHeight

switch (placement) {
case 'bottom':
Expand Down