Enable the `no-unused-vars` ESLint rule #7972

Snuffleupagus · 2017-01-19T13:22:15Z

Please see http://eslint.org/docs/rules/no-unused-vars; note that this patch purposely uses the same rule options as in mozilla-central, such that it fixes part of issue #7957.

It wasn't, in my opinion, entirely straightforward to enable this rule compared to the already existing rules. In many cases a var descriptiveName = ... format was used (more or less) to document the code, and I choose to place the old variable name in a trailing comment to not lose that information.
In other cases I simply commented out the "unused" lines instead of outright removing them, since that's already done in some places in the code-base to preserve the logic for future use. (Since comments are stripped by the preprocessor anyway, that shouldn't be a problem IMHO.)

I welcome feedback on these changes, since it wasn't always entirely easy to know what changes made the most sense in every situation.

timvandermeij · 2017-01-19T21:53:21Z

The trailing comments solution makes sense, but in my opinion commenting out code should be avoided at all costs. I have seen it in other projects before and for me it adds nothing but confusion about why it's commented out. Is the code buggy? Is it intended for future use? It is just unused? I think that for all these questions the answer should be the same: remove the code; we have version control for that. Buggy or unused code just adds noise and makes the code less readable. Preserving logic for future use should not be done in the main tree. It should be developed on a branch and only merged if it's actually used. Moreover, removed code can always be recovered because of version control, in the rare case that one would want to use it later on.

Personally I'd rather not enable this rule and have cleaner code (only removing the "really" unused variables like https://github.com/mozilla/pdf.js/pull/7972/files#diff-18ca06e2bd5be4c9132b51f78388b8f0L41) than enable this rule and having to deal with commented out code, but again, that's my opinion and those of others may differ.

Snuffleupagus · 2017-01-19T22:08:33Z

Unfortunately we've got "prior art" when it comes to keeping commented-out code around, note in particular src/core/evaluator.js#L1326-L1336, which is why I figured it might be OK in moderation. (E.g. the existing commented-out code actually required all the evaluator.js changes made in this patch).

Edit: Obviously, one solution is to simply remove all the code that this patch comment-out, however I didn't want to use such a heavy-handed approach for a first version of a patch.

timvandermeij · 2017-01-19T22:30:37Z

You're right, and that is exactly why I don't think we should add more. I have no idea what to make of that commented out piece of code. Why is it commented out? Will it ever be used again? It just results in more questions than answers, really.

However, I do see why you chose this approach for the patch, given the fact that commented out is already present in the codebase. Therefore, please note that this is purely my opinion on the matter and not critique towards this specific patch!

Snuffleupagus · 2017-01-20T09:38:25Z

Therefore, please note that this is purely my opinion on the matter and not critique towards this specific patch!

I'm not exactly happy about the current situation either; so no worries, I understand what you meant here :-)

@yurydelendik Given that no-unused-vars is one of the rules you specifically mentioned in #7957 (comment), how do you think that we should proceed here?
Can we add even more commented-out code, or should we just remove those lines instead?

yurydelendik · 2017-01-20T15:51:24Z

🤔

Snuffleupagus · 2017-01-25T15:54:01Z

In case it helps: The latest version of the PR only comments out code in evaluator.js, which was necessitated by pre-existing commented-out code, please refer to evaluator.js#L1326-L1336.

That code was added in PR #4570 and it apparently landed in its current (commented-out) state, meaning that this code has never actually been used; see https://github.com/mozilla/pdf.js/pull/4570/files#diff-0b94c2e77a5259f7a728122fdbf9f46aR785.

brendandahl · 2017-01-25T17:30:16Z

I'd say remove commented out code, unless it's in a jsdoc as an example. As for

pdf.js/src/core/evaluator.js

Line 1326 in e0a92a7

// The following will calculate the x and y of the individual glyphs.

we should move that to an issue for when it's needed (we may already have one?).

Snuffleupagus · 2017-01-25T19:15:49Z

I'd say remove commented out code, unless it's in a jsdoc as an example. As for

pdf.js/src/core/evaluator.js

Line 1326 in e0a92a7

// The following will calculate the x and y of the individual glyphs.

we should move that to an issue for when it's needed (we may already have one?).

@brendandahl Thanks so much for the feedback here!
I've removed that code in this PR, and opened issue #7996 for tracking (potentially) using the code again.

timvandermeij

Looks good overall, but I need some more information before approving this. Thanks.

timvandermeij · 2017-01-29T21:23:59Z

test/driver.js

@@ -673,3 +673,5 @@ var Driver = (function DriverClosure() {

  return Driver;
 })();
+
+exports.Driver = Driver;


I don't really understand this change. There is no exports variable in this file. Could you explain this a bit?

Without any changes to this file, the no-unused-vars rule would complain since Driver isn't called from within this file (and this seemed better than adding a bunch more // eslint-disable-line no-unused-vars).

Please keep in mind here that this file, as with most/all test files, are run using Node.js where exports is defined. Note how https://github.com/mozilla/pdf.js/blob/master/test/.eslintrc lists node as one of the environments, and test files such as e.g. https://github.com/mozilla/pdf.js/blob/master/test/downloadutils.js#L170-L171 already uses exports.

I really ought to have mentioned this since it's not that obvious, but hopefully the above clears it up!

timvandermeij · 2017-01-29T21:24:12Z

test/font/fontutils.js

@@ -88,3 +88,7 @@ function verifyTtxOutput(output) {
    throw m[1];
  }
 }
+
+exports.decodeFontData = decodeFontData;


I don't really understand these changes. There is no exports variable in this file. Could you explain this a bit?

Please see #7972 (comment).

timvandermeij · 2017-01-29T21:24:35Z

test/unit/testreporter.js

@@ -74,3 +74,5 @@ var TestReporter = function(browser, appPath) {
    setTimeout(sendQuitRequest, 500);
  };
 };
+
+exports.TestReporter = TestReporter;


I don't really understand this change. There is no exports variable in this file. Could you explain this a bit?

Please see #7972 (comment).

timvandermeij · 2017-01-29T21:25:30Z

test/unit/network_spec.js

@@ -94,7 +94,7 @@ describe('network', function() {
        rangeChunkSize: 65536,
        disableStream: false,
      },
-      disableRange: false
+      disableRange: true


Why is this change made? It looks unrelated to the patch. The same goes for the other changes in this file. Could you explain this a bit?

Well, I suppose that you're correct about this being unrelated!
Since I needed to make a couple of adjustments in this file anyway, I figured that it couldn't hurt to clean up a couple of things. But your comment makes me realize that I may have gone overboard here, so I'll revert this to the essential changes only. Thank you!

Please note that in the updated patch, there's no functional changes.
However, in order to keep the same general structure of all the tests, I've added some currently missing expect statements and the same Promise handling throughout, rather than removing code. I hope this is deemed acceptable!

Please see http://eslint.org/docs/rules/no-unused-vars; note that this patch purposely uses the same rule options as in `mozilla-central`, such that it fixes part of issue 7957. It wasn't, in my opinion, entirely straightforward to enable this rule compared to the already existing rules. In many cases a `var descriptiveName = ...` format was used (more or less) to document the code, and I choose to place the old variable name in a trailing comment to not lose that information. I welcome feedback on these changes, since it wasn't always entirely easy to know what changes made the most sense in every situation.

pdfjsbot · 2017-02-01T09:39:17Z

From: Bot.io (Linux)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.21.233.14:8877/b46ea6964ff2f10/output.txt

pdfjsbot · 2017-02-01T09:39:17Z

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://107.22.172.223:8877/aba0ee5c54deb11/output.txt

pdfjsbot · 2017-02-01T10:04:40Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/b46ea6964ff2f10/output.txt

Total script time: 25.39 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-02-01T10:05:10Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/aba0ee5c54deb11/output.txt

Total script time: 25.89 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

timvandermeij · 2017-02-01T22:10:49Z

/botio-linux preview

pdfjsbot · 2017-02-01T22:10:50Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 1

Live output at: http://107.21.233.14:8877/dab1c181f21cd26/output.txt

pdfjsbot · 2017-02-01T22:13:46Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/dab1c181f21cd26/output.txt

Total script time: 2.28 mins

Published

timvandermeij · 2017-02-01T22:50:37Z

Looks good. Thank you for working on this!

Enable the `no-unused-vars` ESLint rule

Snuffleupagus added the other label Jan 19, 2017

Snuffleupagus requested a review from yurydelendik January 20, 2017 09:38

Snuffleupagus mentioned this pull request Jan 25, 2017

Calculating the position of individual glyphs in getTextContent #7996

Closed

timvandermeij reviewed Jan 29, 2017

View reviewed changes

timvandermeij merged commit 6f0cf8c into mozilla:master Feb 1, 2017

timvandermeij removed the request for review from yurydelendik February 1, 2017 22:50

Snuffleupagus deleted the eslint_no-unused-vars branch February 2, 2017 11:44

Snuffleupagus mentioned this pull request Feb 5, 2017

Prevent browser console errors during testing #8028

Merged

Snuffleupagus mentioned this pull request Nov 3, 2017

Fix issues found by lgtm #9098

Merged

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#7972 from Snuffleupagus/eslint_no-unused-vars

e0b435d

Enable the `no-unused-vars` ESLint rule

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable the `no-unused-vars` ESLint rule #7972

Enable the `no-unused-vars` ESLint rule #7972

Snuffleupagus commented Jan 19, 2017 •

edited

Loading

timvandermeij commented Jan 19, 2017 •

edited

Loading

Snuffleupagus commented Jan 19, 2017 •

edited

Loading

timvandermeij commented Jan 19, 2017 •

edited

Loading

Snuffleupagus commented Jan 20, 2017

yurydelendik commented Jan 20, 2017

Snuffleupagus commented Jan 25, 2017 •

edited

Loading

brendandahl commented Jan 25, 2017

Snuffleupagus commented Jan 25, 2017 •

edited

Loading

timvandermeij left a comment

timvandermeij Jan 29, 2017

Snuffleupagus Jan 29, 2017

timvandermeij Jan 29, 2017

Snuffleupagus Jan 29, 2017

timvandermeij Jan 29, 2017

Snuffleupagus Jan 29, 2017

timvandermeij Jan 29, 2017

Snuffleupagus Jan 29, 2017

Snuffleupagus Jan 29, 2017 •

edited

Loading

pdfjsbot commented Feb 1, 2017

pdfjsbot commented Feb 1, 2017

pdfjsbot commented Feb 1, 2017

pdfjsbot commented Feb 1, 2017

timvandermeij commented Feb 1, 2017

pdfjsbot commented Feb 1, 2017

pdfjsbot commented Feb 1, 2017

timvandermeij commented Feb 1, 2017

Enable the no-unused-vars ESLint rule #7972

Enable the no-unused-vars ESLint rule #7972

Conversation

Snuffleupagus commented Jan 19, 2017 • edited Loading

timvandermeij commented Jan 19, 2017 • edited Loading

Snuffleupagus commented Jan 19, 2017 • edited Loading

timvandermeij commented Jan 19, 2017 • edited Loading

Snuffleupagus commented Jan 20, 2017

yurydelendik commented Jan 20, 2017

Snuffleupagus commented Jan 25, 2017 • edited Loading

brendandahl commented Jan 25, 2017

Snuffleupagus commented Jan 25, 2017 • edited Loading

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

pdfjsbot commented Feb 1, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 1, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 1, 2017

From: Bot.io (Linux)

Success

pdfjsbot commented Feb 1, 2017

From: Bot.io (Windows)

Success

timvandermeij commented Feb 1, 2017

pdfjsbot commented Feb 1, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Feb 1, 2017

From: Bot.io (Linux)

Success

Published

timvandermeij commented Feb 1, 2017

Enable the `no-unused-vars` ESLint rule #7972

Enable the `no-unused-vars` ESLint rule #7972

Snuffleupagus commented Jan 19, 2017 •

edited

Loading

timvandermeij commented Jan 19, 2017 •

edited

Loading

Snuffleupagus commented Jan 19, 2017 •

edited

Loading

timvandermeij commented Jan 19, 2017 •

edited

Loading

Snuffleupagus commented Jan 25, 2017 •

edited

Loading

Snuffleupagus commented Jan 25, 2017 •

edited

Loading

Snuffleupagus Jan 29, 2017 •

edited

Loading