913060 - pretty print should use devtools.editor.tabsize

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This specifies the number of spaces to indent by, so using it would be consistent with how you would write code in the scratchpad otherwise.

Comment 1

[Eldmannen]

Douglas Crockford advocates four spaces.
Google and GitHub uses two spaces.
jQuery uses tabs.
Firefox built-in jsbeautifier defaults to two spaces.
The built-in prettyprinter in Chrome and Internet Explorer uses four spaces.

Comment 2

[mjh563]

Sorry for the dumb question, but what/where is 'pretty print'?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

A recent addition to the scratchpad in Nightly. Adds a button labeled "Pretty Print" that formats the code inside the scratchpad nicelt.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

*nicely

Comment 5

[Sunny [:darkowlzz]]

Attachment: Passed devtools.editor.tabsize to escodegen to maintain uniformity in Devtool PrettyPrinter.

This patch works for me. Please verify the same.

Thanks!

Comment 6

[Sunny [:darkowlzz]]

Attachment: Passed devtools.editor.tabsize to escodegen to maintain uniformity in Devtool PrettyPrinter.

Removed whitespace and corrected the tabsize (incremented by 1).

:fitzgen, could you help in finding out which one is more preferable, `Array(tabsize).join(" ")` or `" ".repeat(tabsize)` ?

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 802437 [details] [diff] [review]
Passed devtools.editor.tabsize to escodegen to maintain uniformity in Devtool PrettyPrinter.

Review of attachment 802437 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good! Can you also add a test that sets `devtools.editor.tabsize` to a non-default value (eg 3) and then verify that the pretty print indents by that amount of spaces? Thanks :)

::: browser/devtools/scratchpad/scratchpad.js
@@ +534,5 @@
>        const ast = Reflect.parse(uglyText);
> +      const prettyText = escodegen.generate(ast, {
> +        format: {
> +          indent: {
> +            style: Array(tabsize + 1).join(" ")

Let's use `String.prototype.repeat`.

Comment 8

[Sunny [:darkowlzz]]

Attachment: Passed devtools.editor.tabsize to escodegen to maintain uniformity in Devtool PrettyPrinter.

Replaced `Array` by `repeat` and added a test.

Please check if the test is good enough.

Comment 9

[mjh563]

Comment on attachment 804901 [details] [diff] [review]
Passed devtools.editor.tabsize to escodegen to maintain uniformity in Devtool PrettyPrinter.

Review of attachment 804901 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I've noticed a couple of small formatting errors:

::: browser/devtools/scratchpad/test/Makefile.in
@@ +31,5 @@
>  		browser_scratchpad_bug807924_cannot_convert_to_string.js \
>  		browser_scratchpad_long_string.js \
>  		browser_scratchpad_open_error_console.js \
>  		browser_scratchpad_pprint.js \
> +    browser_scratchpad_pprint-02.js \

Nit: please correct the indentation on this line.

::: browser/devtools/scratchpad/test/browser_scratchpad_pprint-02.js
@@ +8,5 @@
> +
> +  gBrowser.selectedTab = gBrowser.addTab();
> +  gBrowser.selectedBrowser.addEventListener("load", function onLoad() {
> +    gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
> +    openScratchpad(runTests);  

Nit: trailing whitespace.

@@ +11,5 @@
> +    gBrowser.selectedBrowser.removeEventListener("load", onLoad, true);
> +    openScratchpad(runTests);  
> +  }, true);
> +
> +  content.location = "data:text/html;charset=utf8,test Scatchpad pretty print.";

Typo: Scratchpad not Scatchpad.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 804901 [details] [diff] [review]
Passed devtools.editor.tabsize to escodegen to maintain uniformity in Devtool PrettyPrinter.

Review of attachment 804901 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Once you fix the whitespace/typos/style nits already pointed out, re-upload the patch and add [checkin-needed] to the whiteboard so someone can check it in before merge happens!

Comment 11

[Sunny [:darkowlzz]]

Attachment: Passed devtools.editor.tabsize to escodegen to maintain uniformity in Devtool PrettyPrinter.

Fixed aforementioned nits.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5cfd1ce18d07

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cfd1ce18d07