1375220 - Ensure all if-else statements have curly braces

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P5)

Description

[Edouard Oger [:eoger]]

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

Comment 1

[abhivsharma8]

I would like to take this up. Any suggestion of how I can get started. I am a beginner.

Comment 2

[Edouard Oger [:eoger]]

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.

Comment 3

[Jatin Yadav]

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.

Comment 4

[Edouard Oger [:eoger]]

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

[Jatin Yadav]

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

[Mark Hammond [:markh] [:mhammond]]

(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

[hanbin.chang]

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

[sharu]

Hi,
This is my first bug. I would like to work on this. I have my vm environment ready.

Comment 9

[Edouard Oger [:eoger]]

See comment 2 and 4

Comment 10

[[:Towkir] Ahmed]

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 ?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 12

[[:Towkir] Ahmed]

Attachment: 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) {
    ........
}

Comment 13

[Edouard Oger [:eoger]]

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).

Comment 14

[[:Towkir] Ahmed]

Attachment: curly-braces.patch

Hi, thanks for your feedback, I have updated the patch and hope this work now.
Cheers !

Comment 15

[Edouard Oger [:eoger]]

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?

Comment 16

[Edouard Oger [:eoger]]

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

[Gauri]

Is there any way I can work on this?

Comment 18

[Edouard Oger [:eoger]]

Let's give Ahmed a few weeks to work on his patch first.

Comment 19

[Preeti[:preeti]]

Hello everyone! I am a newbie to Bugzilla and would like to take up this issue. But I am facing problems while I am trying to set up the environment. Can someone please help me out?
This is a part of the error message:
 0:13.20 ERROR: Could not find LLVM/Clang installation for compiling stylo build-time
 0:13.20 bindgen.  Please specify the 'LLVM_CONFIG' environment variable
 0:13.20 (recommended), pass the '--with-libclang-path' and '--with-clang-path'
 0:13.20 options to configure, or put 'llvm-config' in your PATH.  Altering your
 0:13.20 PATH may expose 'clang' as well, potentially altering your compiler,
 0:13.20 which may not be what you intended.
 0:13.23 *** Fix above errors and then restart with\
 0:13.23                "/usr/bin/make -f client.mk build"
 0:13.23 client.mk:123: recipe for target 'configure' failed
 0:13.23 make: *** [configure] Error 1

Thanks in advance!

Comment 20

[akurnya]

If this is still open to take, I would like to work on this.
I've built and run the unified repo using `mach build` and `mach run` although there are some tests failing (not with clang or alike);

But I don't find the .node_modules in root to run:
`./node_modules/.bin/eslint --ext .js,.jsm --rule 'curly: error' services`

Comment 21

[akurnya]

ok I ran npm install and then ran
`./node_modules/.bin/eslint --ext .js,.jsm --rule 'curly: error' services` and fixed braces errors.

Running it now gives me no errors as of now.
What're the next steps for someone starting out to send out patch?

Comment 22

[Devika Sugathan]

Hello,

I would like to work on the bug if this is still open.

Comment 23

[Edouard Oger [:eoger]]

To anyone wanting to work on that bug, instructions are in comment 0, feel free to send a patch as we assign the bug to a person only when a patch has been submitted.

Comment 24

[akurnya]

(In reply to Edouard Oger [:eoger] from comment #23)
> To anyone wanting to work on that bug, instructions are in comment 0, feel
> free to send a patch as we assign the bug to a person only when a patch has
> been submitted.

Do I push using mercurial? How do I create a patch?
do i use MozReview(i think it requires certain access level permission)? Following https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

gives me:

~/mozilla/unified$ hg push review
pushing to https://reviewboard-hg.mozilla.org/autoreview
searching for appropriate review repository
redirecting push to https://reviewboard-hg.mozilla.org/gecko
(ignoring public changeset 04dd259d71db in review request)
abort: no non-public changesets left to review
(add or change the -r argument to include draft changesets)

Comment 25

[Devika Sugathan]

Attachment: Curly--braces.patch

I have attached the patch for this bug.Kindly review the same and give your feedback.

Comment 26

[Eric Rahm [:erahm]]

Comment on attachment 8993916 [details] [diff] [review]
Curly--braces.patch

Hi Devika, thanks for the patch! It looks like it's not quite formatted correctly. Can you attach a patch generated from within the mozilla-central directory:

> cd mozilla-central
> hg export -g . > curly_braces.patch

As far as your patch goes, please make sure you have spaces between your braces,  closing braces are on a new line, and else statements are on the same line as the previous closing brace, ie:

> if (thing) {
>   do_thing();
> } else {
>   do_other_thing();
> }

When your patch is ready, please request review from [e-mail]. No need for sec-approval and please to r+ your own patch.

Comment 27

[Devika Sugathan]

Attachment: curly_braces.patch

Kindly review the changes I have made.

Comment 28

[Edouard Oger [:eoger]]

Comment on attachment 8993997 [details] [diff] [review]
curly_braces.patch

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

Thank you for the patch.
Please set the review flag to ? instead of + when asking for review.
I haven't reviewed the whole patch, I'm seeing a lot of unwarranted indentation changes, please revert these back.
Also please make sure that ./mach eslint succeeds before sending your patch.

::: services/sync/modules-testing/fxa_utils.js
@@ +19,4 @@
>    let requestLog = Log.repository.getLogger("testing.mock-rest");
>    if (!requestLog.appenders.length) { // might as well see what it says :)
>      requestLog.addAppender(new Log.DumpAppender());
> +  }

This looks wrong

::: services/sync/modules/engines.js
@@ +349,3 @@
>        await this.remove(record);
> +    }
> +    else if (!(await this.itemExists(record.id))){

Please have the } and else on the same line

::: services/sync/modules/engines/bookmarks.js
@@ -104,3 @@
>      return clear;
>    },
> -

Keep these lines

@@ -269,4 @@
>  };
>  
>  Utils.deferGetSet(BookmarkSeparator, "cleartext", "pos");
> -

Keep these lines

@@ +425,4 @@
>        throw ex;
>      }
>    },
> +async _syncFinish() {

Do not change indentation here

@@ +449,4 @@
>    async pullAllChanges() {
>      return this.pullNewChanges();
>    },
> +async trackRemainingChanges() {

Ditto

@@ -483,4 @@
>      return new BookmarkValidator();
>    }
>  };
> -

Ditto

@@ -492,4 @@
>  function BookmarksEngine(service) {
>    BaseBookmarksEngine.apply(this, arguments);
>  }
> -

Ditto

@@ +498,4 @@
>      let lastSync = await PlacesSyncUtils.bookmarks.getLastSync();
>      return lastSync;
>    },
> +async setLastSync(lastSync) {

Ditto

@@ +506,4 @@
>    emptyChangeset() {
>      return new BookmarksChangeset();
>    },
> +async _buildGUIDMap() {

Ditto

@@ +517,4 @@
>          }
>        }
>      }
> +let maybeYield = Async.jankYielder();

Ditto

@@ +688,4 @@
>        this._log.error("_shouldReviveRemotelyDeletedRecord called on unmodified item: " + item.id);
>        return false;
>      }
> +  // In addition to preventing the deletion of this record (handled by the caller),

?

::: services/sync/services-sync.js
@@ +48,4 @@
>  pref("services.sync.log.appender.file.logOnError", true);
>  #if defined(NIGHTLY_BUILD)
>  pref("services.sync.log.appender.file.logOnSuccess", true);
> +#else {

This is probably invalid

Comment 29

[Devika Sugathan]

Attachment: Curly_Braces.patch

Comment 30

[Edouard Oger [:eoger]]

Comment on attachment 8994297 [details] [diff] [review]
Curly_Braces.patch

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

It seems that this patch has removed all the interesting parts of the previous patch iteration and instead added a newline at the beginning of a bunch of files.
Please make a quick check and skim through your diff file before submitting it, thanks!

Comment 31

[Devika Sugathan]

I'm sorry for the changes as I didn't know about diff file. Should I submit a new patch by reverting all the changes?

Comment 32

[Devika Sugathan]

Attachment: CURLY_bRACES.patch

Kindly review the following changes

Comment 33

[Edouard Oger [:eoger]]

Comment on attachment 8995600 [details] [diff] [review]
CURLY_bRACES.patch

I'm still seeing a bunch of newlines.

Comment 34

[Devika Sugathan]

(In reply to Edouard Oger [:eoger] from comment #33)
> Comment on attachment 8995600 [details] [diff] [review]
> CURLY_bRACES.patch
> 
> I'm still seeing a bunch of newlines.

Can you please point out the mistake in which I should work on.

Comment 35

[Devika Sugathan]

Attachment: Modified.patch

Comment 36

[Edouard Oger [:eoger]]

Comment on attachment 8996331 [details] [diff] [review]
Modified.patch

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

This is looking way better thank you, I've left comments.

::: services/sync/modules/record.js
@@ +183,3 @@
>        throw new Error(
>            `Record id mismatch: ${this.cleartext.id} != ${this.id}`);
> +        }

Is this the right indentation?

::: services/sync/services-sync.js
@@ +48,4 @@
>  pref("services.sync.log.appender.file.logOnError", true);
>  #if defined(NIGHTLY_BUILD)
>  pref("services.sync.log.appender.file.logOnSuccess", true);
> +else {

These C(?) pre-processor instructions don't need braces

::: services/sync/tps/extensions/tps/bootstrap.js
@@ +64,2 @@
>          return;
> +      }

These indentations don't look right in that file I think

::: services/sync/tps/extensions/tps/resource/logger.jsm
@@ +83,3 @@
>        throw new Error("ASSERTION FAILED! " + msg + "; expected " +
>              JSON.stringify(val2) + ", got " + JSON.stringify(val1));
> +          }

That indentation looks wrong

::: services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm
@@ +375,3 @@
>          PlacesUtils.annotations.removeItemAnnotation(itemId,
>                                           "bookmarkProperties/description");
> +                                       }

This doesn't look right (indentation)

@@ +479,3 @@
>        PlacesUtils.annotations.removeItemAnnotation(itemId,
>                                         "bookmarkProperties/loadInSidebar");
> +                                     }

Ditto

@@ +605,4 @@
>        loadInSidebar = PlacesUtils.annotations.getItemAnnotation(
>          itemId,
>          "bookmarkProperties/loadInSidebar");
> +      }

Ditto

::: services/sync/tps/extensions/tps/resource/modules/tabs.jsm
@@ +83,3 @@
>              return true;
> +          }
> +          }

Ditto

::: services/sync/tps/extensions/tps/resource/tps.jsm
@@ +519,3 @@
>              Logger.logInfo("executing action " + action.toUpperCase() +
>                             " on bookmark " + JSON.stringify(bookmark));
> +                         }

Indentation

Comment 37

[Devika Sugathan]

Attachment: new.patch

Comment 38

[Edouard Oger [:eoger]]

Comment on attachment 8997008 [details] [diff] [review]
new.patch

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

Almost there! I left 2 small comments.

::: services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm
@@ +376,2 @@
>                                           "bookmarkProperties/description");
> +                                       }

Indentation problem here

@@ +481,2 @@
>                                         "bookmarkProperties/loadInSidebar");
> +                                     }

Ditto here

Comment 39

[Devika Sugathan]

Attachment: Curlyl.patch

Comment 40

[Edouard Oger [:eoger]]

Comment on attachment 8997129 [details] [diff] [review]
Curlyl.patch

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

This is looking great, but doesn't apply correctly on mozilla-central. Could you rebase the patch?

Comment 41

[Devika Sugathan]

Attachment: Braces.patch

Comment 42

[Edouard Oger [:eoger]]

Comment on attachment 8997478 [details] [diff] [review]
Braces.patch

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

I'm still getting merging conflicts with your patch:
- services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm
- services/sync/tps/extensions/tps/resource/modules/tabs.jsm.rej

Please rebase your patch against the latest mozilla-central, thanks.

Comment 43

[Devika Sugathan]

Attachment: braces.patch

Comment 44

[Devika Sugathan]

Attachment: Rebased.patch

Comment 45

[Edouard Oger [:eoger]]

Comment on attachment 8997884 [details] [diff] [review]
Rebased.patch

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

This looks great, thank you!

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e062ff4f63b
Ensure all if-else statements have curly braces r=eoger

Comment 47

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9e062ff4f63b