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

Remove unused vars #8954

Merged
merged 17 commits into from
Sep 17, 2014
Merged

Remove unused vars #8954

merged 17 commits into from
Sep 17, 2014

Conversation

@marcelgerber
Copy link
Contributor

@marcelgerber 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

peterflynn
peterflynn reviewed Sep 4, 2014
View reviewed changes
src/command/Menus.js
Copy link
Member

@peterflynn peterflynn Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@marcelgerber marcelgerber Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@le717 le717 mentioned this pull request Sep 4, 2014
Merged
@marcelgerber
Copy link
Contributor Author

marcelgerber commented Sep 4, 2014

Fixed the Travis build.

le717
le717 reviewed Sep 4, 2014
View reviewed changes
src/extensions/default/HtmlEntityCodeHints/unittests.js
Copy link
Contributor

@le717 le717 Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@redmunds redmunds Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@marcelgerber marcelgerber Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Merge remote-tracking branch 'upstream/master' into unused-vars
98a79c1 
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
@marcelgerber marcelgerber force-pushed the unused-vars branch 2 times, most recently from 019eeb4 to 006967b Compare September 13, 2014 07:24
@ingorichter ingorichter self-assigned this Sep 15, 2014
@ingorichter
Copy link
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.

dangoor
dangoor reviewed Sep 16, 2014
View reviewed changes
src/language/HTMLInstrumentation.js Outdated
Copy link
Contributor

@dangoor dangoor Sep 16, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be removed. It's a handy debugging function.

@marcelgerber
Copy link
Contributor Author

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
Copy link
Contributor Author

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 marcelgerber reopened this Sep 16, 2014
@marcelgerber
Copy link
Contributor Author

marcelgerber commented Sep 16, 2014

(Didn't mean to close)

@ingorichter
Copy link
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
Copy link
Contributor Author

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
Merge pull request #8954 from MarcelGerber/unused-vars
6affa79 
Remove unused vars
@ingorichter ingorichter merged commit 6affa79 into adobe:master Sep 17, 2014
@ingorichter
Copy link
Contributor

ingorichter commented Sep 17, 2014

Getting ready to rumble...

@marcelgerber marcelgerber deleted the unused-vars branch September 17, 2014 16:29
@core-ai-bot core-ai-bot mentioned this pull request Aug 30, 2021
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@ingorichter ingorichter

Labels

fix in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@marcelgerber @ingorichter @dangoor @redmunds @peterflynn @le717