Ensure all if-else statements have curly braces

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P5
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: eoger, Assigned: devikasugathan007, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 11 obsolete attachments)

(Reporter)

Description

2 years 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

2 years ago
Mentor: eoger
Keywords: good-first-bug
Priority: -- → P5

Comment 1

2 years 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

2 years 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 years 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 years 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 years 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.
(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 years 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 years ago
Hi,
This is my first bug. I would like to work on this. I have my vm environment ready.
(Reporter)

Comment 9

2 years ago
See comment 2 and 4

Comment 10

2 years 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)

Comment 12

2 years ago
Posted patch curly-braces.patch (obsolete) — Splinter Review
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

2 years 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+

Comment 14

2 years ago
Posted patch curly-braces.patch (obsolete) — Splinter Review
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

2 years 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

2 years 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

2 years ago
Is there any way I can work on this?
(Reporter)

Comment 18

2 years ago
Let's give Ahmed a few weeks to work on his patch first.
Assignee: 3ugzilla → nobody
Status: ASSIGNED → NEW
Whiteboard: [lang=js]

Comment 19

9 months ago
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

9 months ago
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`
Flags: needinfo?(eoger)

Comment 21

9 months ago
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?
Flags: needinfo?(eoger)
(Assignee)

Comment 22

9 months ago
Hello,

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

Comment 23

9 months ago
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

9 months ago
(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)
Flags: needinfo?(eoger)
(Assignee)

Comment 25

9 months ago
Posted patch Curly--braces.patch (obsolete) — Splinter Review
I have attached the patch for this bug.Kindly review the same and give your feedback.
Attachment #8993916 - Flags: sec-approval?
Attachment #8993916 - Flags: review+
Attachment #8993916 - Flags: feedback+
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 eoger@fastmail.com. No need for sec-approval and please to r+ your own patch.
Attachment #8993916 - Flags: review+
Attachment #8993916 - Flags: feedback+
Attachment #8993916 - Flags: sec-approval?
(Assignee)

Comment 27

9 months ago
Posted patch curly_braces.patch (obsolete) — Splinter Review
Kindly review the changes I have made.
Attachment #8993997 - Flags: review+
Attachment #8993997 - Flags: feedback+
(Reporter)

Comment 28

9 months ago
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
Attachment #8993997 - Flags: review-
Attachment #8993997 - Flags: review+
Attachment #8993997 - Flags: feedback+
(Assignee)

Comment 29

9 months ago
Posted patch Curly_Braces.patch (obsolete) — Splinter Review
Attachment #8994297 - Flags: review?(eoger)
(Reporter)

Comment 30

9 months ago
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!
Attachment #8994297 - Flags: review?(eoger) → review-
(Reporter)

Updated

9 months ago
Flags: needinfo?(eoger)
(Assignee)

Comment 31

9 months ago
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?
(Assignee)

Comment 32

9 months ago
Posted patch CURLY_bRACES.patch (obsolete) — Splinter Review
Kindly review the following changes
Attachment #8995600 - Flags: review?(eoger)
(Reporter)

Comment 33

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

I'm still seeing a bunch of newlines.
Attachment #8995600 - Flags: review?(eoger) → review-
(Assignee)

Comment 34

9 months ago
(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.
(Assignee)

Updated

9 months ago
Attachment #8993916 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8995600 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8993997 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8994297 - Attachment is obsolete: true
(Assignee)

Comment 35

9 months ago
Posted patch Modified.patch (obsolete) — Splinter Review
Attachment #8996331 - Flags: review?(eoger)
(Reporter)

Comment 36

9 months ago
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
Attachment #8996331 - Flags: review?(eoger)
(Assignee)

Comment 37

9 months ago
Posted patch new.patch (obsolete) — Splinter Review
Attachment #8997008 - Flags: review?(eoger)
(Reporter)

Comment 38

9 months ago
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
Attachment #8997008 - Flags: review?(eoger)
(Assignee)

Comment 39

9 months ago
Posted patch Curlyl.patch (obsolete) — Splinter Review
Attachment #8997129 - Flags: review?(eoger)
(Reporter)

Comment 40

9 months ago
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?
Attachment #8997129 - Flags: review?(eoger)
(Assignee)

Comment 41

9 months ago
Posted patch Braces.patch (obsolete) — Splinter Review
Attachment #8997478 - Flags: review?(eoger)
(Reporter)

Comment 42

9 months ago
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.
Attachment #8997478 - Flags: review?(eoger)
(Assignee)

Comment 43

9 months ago
Posted patch braces.patch (obsolete) — Splinter Review
Attachment #8997651 - Flags: review?(eoger)
(Assignee)

Updated

9 months ago
Attachment #8996331 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8997008 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Flags: needinfo?(eoger)
(Assignee)

Comment 44

9 months ago
Posted patch Rebased.patchSplinter Review
Attachment #8997884 - Flags: review?(eoger)
Attachment #8997884 - Flags: feedback?
(Reporter)

Comment 45

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

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

This looks great, thank you!
Attachment #8997884 - Flags: review?(eoger)
Attachment #8997884 - Flags: review+
Attachment #8997884 - Flags: feedback?
(Reporter)

Updated

9 months ago
Flags: needinfo?(eoger)
Keywords: checkin-needed

Comment 46

9 months ago
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e062ff4f63b
Ensure all if-else statements have curly braces r=eoger
Keywords: checkin-needed
(Assignee)

Updated

9 months ago
Attachment #8997129 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8997651 - Attachment is obsolete: true
Attachment #8997651 - Flags: review?(eoger)
(Reporter)

Updated

9 months ago
Attachment #8901138 - Attachment is obsolete: true
(Reporter)

Updated

9 months ago
Attachment #8997478 - Attachment is obsolete: true
(Reporter)

Updated

9 months ago
Assignee: nobody → devikasugathan007
Status: NEW → ASSIGNED

Comment 47

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e062ff4f63b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.