Pretty Print should indent switch statement like CodeMirror and MDN Coding Style

RESOLVED FIXED in Firefox 32

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: anaran, Assigned: anaran)

Tracking

32 Branch
Firefox 32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Pretty Print currently does:

switch (x) {
case a:
  foo();
  break;
default:
  bar()
}

CodeMirror Shift-Tab and
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
do and say respectively:

switch (x) {
  case a:
    foo();
    break;
  default:
    bar()
}

I am separating my implementation from Bug 975477 comment 5 now in the hope they will be more acceptable separately.
(Assignee)

Comment 1

4 years ago
Created attachment 8432125 [details] [diff] [review]
0001-Bug-1018586-Pretty-Print-should-indent-switch-statem.patch
Attachment #8432125 - Flags: review?(nfitzgerald)
We don't use the mdn coding style in devtools, but I'll take a look at the code and if it doesn't complicate things too much I'll r+.
Comment on attachment 8432125 [details] [diff] [review]
0001-Bug-1018586-Pretty-Print-should-indent-switch-statem.patch

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

Can't see enough context, when you upload patches make sure you format them with 8 lines of context. See the -U 8 stuff here: https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

However, in this special case where pretty-fast is an upstream project, can you just submit PRs there and then I can just merge upstream when PRs land.
Attachment #8432125 - Flags: review?(nfitzgerald)
Needs test(s) as well.
(Assignee)

Comment 5

4 years ago
OK, I'll submit a PR with test.
(Assignee)

Comment 6

4 years ago
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> Needs test(s) as well.

Oh wait, there is an existing switch indentation test that will fail with my patch.
Shouldn't I just update that and be done with the test coverage?
Sounds good.
(Assignee)

Comment 8

4 years ago
Created attachment 8432726 [details] [review]
Upstream pull request, with updated test coveralge
Attachment #8432125 - Attachment is obsolete: true
Attachment #8432726 - Flags: review?(nfitzgerald)
Depends on: 1020587
Comment on attachment 8432726 [details] [review]
Upstream pull request, with updated test coveralge

Created bug 1020587 to update pretty-fast in the tree.
Attachment #8432726 - Flags: review?(nfitzgerald)
Resolved as part of https://hg.mozilla.org/mozilla-central/rev/e157171a4df1
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Assignee: nobody → adrian
Target Milestone: --- → Firefox 32

Updated

2 years ago
See Also: → bug 1261971
You need to log in before you can comment on or make changes to this bug.