New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused vars #8954

Merged
merged 17 commits into from Sep 17, 2014

Conversation

Projects
None yet
6 participants
@MarcelGerber
Contributor

MarcelGerber commented Sep 3, 2014

Code cleanup for JSHint support

  • Removes unused vars (reported by JSHint)
  • Adjusts .jshintrc config: Remove obsolete prefs, add unused: "vars" (... will change our Travis results)
  • Remove unused /* globals */ (reported by JSHint)
  • Make ExtensionLoader.js pass JSLint
@@ -31,8 +31,7 @@ define(function (require, exports, module) {
var _ = require("thirdparty/lodash");
// Load dependent modules
var Global = require("utils/Global"),

This comment has been minimized.

@peterflynn

peterflynn Sep 4, 2014

Member

Drive-by note: we can't just remove unused references to Globals, since this is used to ensure a load order that sets up the global brackets.* members early enough. You'll have to change cases like this to a require() call that's not stored into a variable.

@peterflynn

peterflynn Sep 4, 2014

Member

Drive-by note: we can't just remove unused references to Globals, since this is used to ensure a load order that sets up the global brackets.* members early enough. You'll have to change cases like this to a require() call that's not stored into a variable.

This comment has been minimized.

@MarcelGerber

MarcelGerber Sep 4, 2014

Contributor

Done (MarcelGerber@5c44c87).
I didn't add it back to SidebarView as it never uses brackets.

@MarcelGerber

MarcelGerber Sep 4, 2014

Contributor

Done (MarcelGerber@5c44c87).
I didn't add it back to SidebarView as it never uses brackets.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Sep 4, 2014

Contributor

Fixed the Travis build.

Contributor

MarcelGerber commented Sep 4, 2014

Fixed the Travis build.

@@ -23,15 +23,13 @@
/*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, describe, it, xit, expect, beforeEach, afterEach, waitsFor, runs, $, brackets, waitsForDone */

This comment has been minimized.

@le717

le717 Sep 4, 2014

Contributor

Because a test might have to be disabled at any time, perhaps xit should be added to the globals array in .jshintrc?

@le717

le717 Sep 4, 2014

Contributor

Because a test might have to be disabled at any time, perhaps xit should be added to the globals array in .jshintrc?

This comment has been minimized.

@redmunds

redmunds Sep 4, 2014

Contributor

Yes, but xit should only be used as a temporary fix, so it's nice to get a warning from jslint as a reminder.

@redmunds

redmunds Sep 4, 2014

Contributor

Yes, but xit should only be used as a temporary fix, so it's nice to get a warning from jslint as a reminder.

This comment has been minimized.

@MarcelGerber

MarcelGerber Sep 4, 2014

Contributor

We actually have 28 calls to xit in 10 files on current master.

@MarcelGerber

MarcelGerber Sep 4, 2014

Contributor

We actually have 28 calls to xit in 10 files on current master.

MarcelGerber added some commits Sep 4, 2014

Merge remote-tracking branch 'upstream/master' into unused-vars
Conflicts:
	src/brackets.js
	src/document/DocumentManager.js
	src/editor/EditorManager.js
	src/editor/ImageViewer.js
	src/extensions/default/JavaScriptQuickEdit/unittests.js
	src/extensions/default/UrlCodeHints/unittests.js
	src/language/CodeInspection.js
	src/project/WorkingSetSort.js
	src/project/WorkingSetView.js
	src/search/FindReplace.js
	src/search/ScrollTrackMarkers.js
	test/spec/FindReplace-test.js
	test/spec/LanguageManager-test.js
Merge remote-tracking branch 'upstream/master' into unused-vars
Conflicts:
	src/search/FindReplace.js
	src/utils/ViewUtils.js

@ingorichter ingorichter self-assigned this Sep 15, 2014

@ingorichter

This comment has been minimized.

Show comment
Hide comment
@ingorichter

ingorichter Sep 16, 2014

Contributor

I'm done with my review. There is nothing that made me nervous and I'd like to merge this change.

Contributor

ingorichter commented Sep 16, 2014

I'm done with my review. There is nothing that made me nervous and I'd like to merge this change.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Sep 16, 2014

Contributor

Just did some other changes, mostly whitespace.
For just reviewing the actual changes, please use MarcelGerber@7734b35?w=0.

Contributor

MarcelGerber commented Sep 16, 2014

Just did some other changes, mostly whitespace.
For just reviewing the actual changes, please use MarcelGerber@7734b35?w=0.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Sep 16, 2014

Contributor

Just double checked everything in this PR. All tests pass, JSHint doesn't complain, Travis succeeds.

@ingorichter Please let me know whether I should stash the changes.
And be aware merging this PR will break many open PRs and their Travis builds, and it will make JSHint behave rather different from JSLint.

Contributor

MarcelGerber commented Sep 16, 2014

Just double checked everything in this PR. All tests pass, JSHint doesn't complain, Travis succeeds.

@ingorichter Please let me know whether I should stash the changes.
And be aware merging this PR will break many open PRs and their Travis builds, and it will make JSHint behave rather different from JSLint.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Sep 16, 2014

Contributor

(Didn't mean to close)

Contributor

MarcelGerber commented Sep 16, 2014

(Didn't mean to close)

@ingorichter

This comment has been minimized.

Show comment
Hide comment
@ingorichter

ingorichter Sep 16, 2014

Contributor

@MarcelGerber I'm aware that this will most likely to cause some of the existing PR's to choke. If we need to wait for a window where no PR is open and waiting to be merged, then we''l wait forever. :-)

Contributor

ingorichter commented Sep 16, 2014

@MarcelGerber I'm aware that this will most likely to cause some of the existing PR's to choke. If we need to wait for a window where no PR is open and waiting to be merged, then we''l wait forever. :-)

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Sep 17, 2014

Contributor

@ingorichter That's what I thought as well ;)
Btw, I resolved the conflicts once again.

Contributor

MarcelGerber commented Sep 17, 2014

@ingorichter That's what I thought as well ;)
Btw, I resolved the conflicts once again.

ingorichter added a commit that referenced this pull request Sep 17, 2014

@ingorichter ingorichter merged commit 6affa79 into adobe:master Sep 17, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@ingorichter

This comment has been minimized.

Show comment
Hide comment
@ingorichter

ingorichter Sep 17, 2014

Contributor

Getting ready to rumble...

Contributor

ingorichter commented Sep 17, 2014

Getting ready to rumble...

@MarcelGerber MarcelGerber deleted the MarcelGerber:unused-vars branch Sep 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment