Skip to content
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

Enable the no-nested-ternary ESLint rule in the web/ directory #11488

Merged
merged 1 commit into from
Jan 8, 2020

Conversation

Snuffleupagus
Copy link
Collaborator

This rule is already enabled in mozilla-central, and helps avoid some confusing formatting[1], see https://searchfox.org/mozilla-central/rev/a92ed79b0bc746159fc31af1586adbfa9e45e264/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js#209-210

Since some cases may not be aren't entirely straightforward to convert, and some we probably don't want to change either[2], this patch is limited to the web/ directory as a proof-of-concept.

Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-nested-ternary


[1] One example with three ternary statements:

pdf.js/src/core/fonts.js

Lines 2623 to 2630 in 93aa613

var stackDelta =
op <= 0x8e
? TTOpsStackDeltas[op]
: op >= 0xc0 && op <= 0xdf
? -1
: op >= 0xe0
? -2
: 0;

[2] One example that should be whitelisted:

pdf.js/src/core/jbig2.js

Lines 82 to 92 in 93aa613

var value = readBits(1) ?
(readBits(1) ?
(readBits(1) ?
(readBits(1) ?
(readBits(1) ?
(readBits(32) + 4436) :
readBits(12) + 340) :
readBits(8) + 84) :
readBits(6) + 20) :
readBits(4) + 4) :
readBits(2);

This rule is already enabled in mozilla-central, and helps avoid some confusing formatting[1], see https://searchfox.org/mozilla-central/rev/a92ed79b0bc746159fc31af1586adbfa9e45e264/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js#209-210

Since some cases may not be aren't entirely straightforward to convert, and some we probably don't want to change either[2], this patch is limited to the `web/` directory as a proof-of-concept.

Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-nested-ternary

---
[1] One example with *three* ternary statements: https://github.com/mozilla/pdf.js/blob/93aa613db7a874e6ed964f394241605d5586c142/src/core/fonts.js#L2623-L2630

[2] One example that should be whitelisted: https://github.com/mozilla/pdf.js/blob/93aa613db7a874e6ed964f394241605d5586c142/src/core/jbig2.js#L82-L92
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2020

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/8ab0cee7190606b/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2020

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/8ab0cee7190606b/output.txt

Total script time: 1.71 mins

Published

@timvandermeij timvandermeij merged commit 4729fdc into mozilla:master Jan 8, 2020
@timvandermeij
Copy link
Contributor

I agree with the decision at mozilla-central for enabling this since I also found the nested ternary statements harder to read than the new structure. Thanks!

@Snuffleupagus
Copy link
Collaborator Author

[...] since I also found the nested ternary statements harder to read than the new structure.

At least to my eye, the recent introduction of Prettier actually made some of these cases even less readable than before since any helpful indentation is now forbidden. That's actually what prompted me to start looking into this now, however a full conversion may take some time since it's not always clear immediately what the best solution is (and some cases we'll want to whitelist).

Also, we used to have loads of these nested ternary statements in the Colourspace/Image code, but converting that code to use Uint8ClampedArray got rid of most of those; thanks for landing this!

@Snuffleupagus Snuffleupagus deleted the web-no-nested-ternary branch January 8, 2020 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants