Closed Bug 913060 Opened 6 years ago Closed 6 years ago

pretty print should use devtools.editor.tabsize

Categories

(DevTools :: Scratchpad, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: fitzgen, Assigned: darkowlzz)

Details

(Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][engineering-friend])

Attachments

(1 file, 3 obsolete files)

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.
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js]
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.
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Version: unspecified → Trunk
Sorry for the dumb question, but what/where is 'pretty print'?
A recent addition to the scratchpad in Nightly. Adds a button labeled "Pretty Print" that formats the code inside the scratchpad nicelt.
Assignee: nobody → indiasuny000
Status: NEW → ASSIGNED
This patch works for me. Please verify the same.

Thanks!
Attachment #802405 - Flags: feedback?(nfitzgerald)
Attachment #802405 - Flags: feedback?(mjh563)
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)` ?
Attachment #802405 - Attachment is obsolete: true
Attachment #802405 - Flags: feedback?(nfitzgerald)
Attachment #802405 - Flags: feedback?(mjh563)
Attachment #802437 - Flags: feedback?(nfitzgerald)
Attachment #802437 - Flags: feedback?(mjh563)
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`.
Attachment #802437 - Flags: feedback?(nfitzgerald) → feedback+
Replaced `Array` by `repeat` and added a test.

Please check if the test is good enough.
Attachment #802437 - Attachment is obsolete: true
Attachment #802437 - Flags: feedback?(mjh563)
Attachment #804901 - Flags: feedback?(nfitzgerald)
Attachment #804901 - Flags: feedback?(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.
Attachment #804901 - Flags: feedback?(mjh563) → feedback+
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!
Attachment #804901 - Flags: review+
Attachment #804901 - Flags: feedback?(nfitzgerald)
Fixed aforementioned nits.
Attachment #804901 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][engineering-friend]
https://hg.mozilla.org/integration/fx-team/rev/5cfd1ce18d07
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][engineering-friend] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][engineering-friend][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5cfd1ce18d07
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][engineering-friend][fixed-in-fx-team] → [good first bug][mentor=nfitzgerald@mozilla.com][lang=js][engineering-friend]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.