If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Ensure all if-else statements have curly braces

ASSIGNED
Assigned to

Status

()

Firefox
Sync
P5
normal
ASSIGNED
3 months ago
20 days ago

People

(Reporter: eoger, Assigned: Towkir, Mentored)

Tracking

({good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 months ago
Have a look (not an exhaustive list):
http://searchfox.org/mozilla-central/search?q=%28if+%5C%28.%2B%5C%29%7Celse%29%24&regexp=true&path=services%2Fsync

Fortunately, it seems that eslint could do that work for us using the --fix option.
See http://eslint.org/docs/rules/curly
(Reporter)

Updated

3 months ago
Mentor: eoger@fastmail.com
Keywords: good-first-bug

Updated

3 months ago
Priority: -- → P5

Comment 1

3 months ago
I would like to take this up. Any suggestion of how I can get started. I am a beginner.
Flags: needinfo?(eoger)
(Reporter)

Comment 2

3 months ago
For starters, you need to check out and build Firefox:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Once you have done that, to get you started, here's a command that I used on the root directory that gave me some curly braces violations:

./node_modules/.bin/eslint --ext .js,.jsm --rule 'curly: error' services

You could add the argument --fix to try and let eslint fix these errors by itself, but in my case the formatting was not respected.
Flags: needinfo?(eoger)

Comment 3

2 months ago
I would like to get started on this and take this as my first ever contribution to open source. I read "Edouard Oger"'s comment, but as the current solution doesn't respect formatting. So should I go to every file and fix it my self or should I look for some other smarter alternative?

Thanks.
(Reporter)

Comment 4

2 months ago
This might take you a lot of time to go through every file and fix the indentation (eslint detects 174 problems), also this is a P5 bug, so it is far from urgent.
You should probably try to find a better/easier/more interesting first bug to work on.
If you insist on wanting to work on this bug, then yes I guess you would have to fix everything by hand.

Comment 5

2 months ago
So if I want to give this a try how should I go about proceeding with the process? Where should I get all the files, how should I test that what I have done has not broken the system and everything else.

Thank You.

Comment 6

2 months ago
(In reply to Jatin Yadav from comment #5)
> So if I want to give this a try how should I go about proceeding with the
> process? Where should I get all the files, how should I test that what I
> have done has not broken the system and everything else.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction should have all the info you need.

Comment 7

2 months ago
Hi,
I am a new to Bugzilla communit and I am wondering if this bug is picked up by anyone yet.
If not, I would like to work on it. 
I have all the VM environment set up for testing and development.

Thanks,

Comment 8

2 months ago
Hi,
This is my first bug. I would like to work on this. I have my vm environment ready.
(Reporter)

Comment 9

2 months ago
See comment 2 and 4
(Assignee)

Comment 10

a month ago
Hey, looks like multiple contributor expressed their eagerness in fixing this, is there any specific rule about who will actually do it ? or anyone able to create a correct patch can submit here ?
Flags: needinfo?(eoger)
(In reply to [:Towkir] Ahmed from comment #10)
> Hey, looks like multiple contributor expressed their eagerness in fixing
> this, is there any specific rule about who will actually do it ? or anyone
> able to create a correct patch can submit here ?

We try and give people who express interest a week or so to get a patch up, so it's fine for you to start working on this. Let us know when you have a patch nearly ready to review and we'll assign the bug to you so you can push it to mozreview for review with eoeger as the reviewer.
Flags: needinfo?(eoger)
(Assignee)

Comment 12

a month ago
Created attachment 8899105 [details] [diff] [review]
curly-braces.patch

I have manually added curly braces to all the results of search "(if \(.+\)|else)$" [regexp] within "services/sync" (currently 426)
I edited some function structure from :

function name(p1, p2)
{
    ........
}

to:

function name(p1, p2) {
    ........
}
Assignee: nobody → 3ugzilla
Status: NEW → ASSIGNED
Attachment #8899105 - Flags: review?(eoger)
(Reporter)

Comment 13

a month ago
Comment on attachment 8899105 [details] [diff] [review]
curly-braces.patch

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

Awesome patch Towkir, I wasn't really sure anyone would want to power through this bug since it's super tedious work. Thank you!
I have added two comments, but otherwise it looks all good to me.
We have to make sure all the tests pass before we merge this later. If you don't have access to the try server, don't worry I'll run these tests for you.
Thanks again!

::: services/sync/modules/service.js
@@ +996,3 @@
>          this._log.info("Metadata record not found, server was wiped to ensure " +
>                         "consistency.");
> +      } else {// 200

Add a space before the comment. This will make eslint fail.

::: services/sync/tps/extensions/tps/resource/modules/forms.jsm
@@ +200,3 @@
>          */
>            this.id = formdata.guid;
> +        }

This will give you a syntax error since you don't have a corresponding opening brace (it is commented).
Attachment #8899105 - Flags: review?(eoger) → feedback+
(Assignee)

Comment 14

27 days ago
Created attachment 8901138 [details] [diff] [review]
curly-braces.patch

Hi, thanks for your feedback, I have updated the patch and hope this work now.
Cheers !
Attachment #8899105 - Attachment is obsolete: true
Attachment #8901138 - Flags: review?(eoger)
(Reporter)

Comment 15

27 days ago
Comment on attachment 8901138 [details] [diff] [review]
curly-braces.patch

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

Hi Ahmed,

I'm still seing some errors if I run:
./node_modules/.bin/eslint --ext .js,.jsm --rule 'curly: error' services
at the root of the repository, could you fix the remaining errors?
Attachment #8901138 - Flags: review?(eoger)
(Reporter)

Comment 16

24 days ago
While we are at it, could you also add |"curly": "error"| to the list of eslint rules in |services/.eslintrc.js|?
This way we won't introduce conditions without curly braces anymore.
It also allows you to test your changes by entering |./mach eslint services/| instead of the line I copy/pasted for you in comment 15.

Comment 17

20 days ago
Is there any way I can work on this?
(Reporter)

Comment 18

20 days ago
Let's give Ahmed a few weeks to work on his patch first.
You need to log in before you can comment on or make changes to this bug.