Trailing whitespace in Sync source code

NEW
Assigned to

Status

()

Firefox
Sync
P3
normal
5 months ago
3 months ago

People

(Reporter: nicolaso, Assigned: Aruna, Mentored)

Tracking

({good-first-bug})

56 Branch
good-first-bug
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 months ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170722112649



Actual results:

There are a handful of places in Sync code with trailing whitespace.

http://searchfox.org/mozilla-central/search?q=%5Cs%2B%24&case=false&regexp=true&path=services%2Fsync

I feel like we could have eslint check those for us. At any rate, it should at least be easy to fix the existing occurrences of trailing whitespace.
(Reporter)

Updated

5 months ago
Component: Untriaged → Sync
Mentor: eoger@fastmail.com
Keywords: good-first-bug
Priority: -- → P3
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

5 months ago
So it appears that all the violations we get are located in the services/sync/tps/extensions/mozmill dir which is explicitly ignored by eslint. I'm not sure there's much more we can do here :)
(Assignee)

Comment 2

5 months ago
Hey!
I would like to pick this as my first bug.Could you shed some light on how I should go about it.
(In reply to Aruna from comment #2)
> Hey!
> I would like to pick this as my first bug.

Awesome! Please see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction for how to get started. Once you've cloned Gecko, remove the trailing whitespace from all the lines mentioned in the Searchfox results in comment 0, commit your changes, and submit a patch to MozReview (https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html).
(Assignee)

Comment 4

5 months ago
I wanted to inquire that is it  very essential that I should do it with Gecko,because I had already set up my environment based on the artifact mode.If it is then I will get straight to it.
(In reply to Aruna from comment #4)
> I had already set up my environment based on the artifact
> mode.

Great! Yes, you can use an artifact build.
(Assignee)

Comment 6

5 months ago
I was looking through the code in the link provided,but couldn't quite understand from where to find the lines mentioned in the SearchFox results.If u could throw some light,it would be great.
Sure. Searchfox is a tool that searches the Firefox source tree; everything there, you have locally in your tree, too. The first result points to lines 56 and 57 in "services/sync/tps/extensions/mozmill/resource/stdlib/EventUtils.js"...so, you'd open that file, remove the trailing whitespace, and repeat for the other 2 files.
(Assignee)

Comment 8

5 months ago
Thank you for explaining me about Searchfox. I have one more issues,I would be very thankful if you could help.How should I edit the given code? i.e how should I copy it to my system,because I am not able to edit it when I open the link.

Comment 9

5 months ago
(In reply to Aruna from comment #8)
> Thank you for explaining me about Searchfox. I have one more issues,I would
> be very thankful if you could help.How should I edit the given code? i.e how
> should I copy it to my system,because I am not able to edit it when I open
> the link.

In comment 4 you mentioned that you have artifact builds working - this means you must have that same source code locally, and that is what you should edit. You might also want to have another look at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction.
(Assignee)

Comment 10

5 months ago
Could you correct me if I am wrong, the other two files are pointed by lines 1185 and 4972
(Assignee)

Comment 11

4 months ago
Hey, 
could you guide me on how to submit a patch
(In reply to Aruna from comment #10)
> Could you correct me if I am wrong, the other two files are pointed by lines
> 1185 and 4972

I'm not sure what you mean by this - lines 1185 and 4972 of what?

(In reply to Aruna from comment #11)
> Hey, 
> could you guide me on how to submit a patch

Please see comment 3 - the introduction guide should tell you everything you need to know, but let us know if something is unclear.

Thanks!
(Assignee)

Comment 13

4 months ago
Hey,
I read through the introduction guide.I need to be an assignee to send a patch.Could someone with bug-editing privileges assign it to me.

Updated

4 months ago
Assignee: nobody → aruna.maurya12

Comment 14

4 months ago
Is this still being worked on?
(In reply to Aruna from comment #13)
> Hey,
> I read through the introduction guide.I need to be an assignee to send a
> patch.Could someone with bug-editing privileges assign it to me.

Hi Aruna,
  Do you still intend submitting a patch here?
Flags: needinfo?(aruna.maurya12)
(Assignee)

Comment 16

4 months ago
Hey,
I have completed the work, but I am stuck on some of the intricacies of making the patch.I fully intend on submitting the patch for this bug, although there will be a slight more delay because of my University exams till 25th of this month.I will get back to work as soon as possible and submit the patch.Inconvenience is deeply regretted.
Flags: needinfo?(aruna.maurya12)
No worries, thanks for getting back to us, and good luck with your exams! Feel free to ask for any help you need putting the patch together.
(Assignee)

Comment 18

4 months ago
I was trying to make the patch,I faced an issue with watchman.After having dealt with it ,I am still getting the message-Local changes found,refresh first!. Could you help me out on this?
I guess you are using Mercurial queues? If so, this probably means that "hg diff" or "hg status" will be showing local changes. If you are already on the correct patch where these changes belong, you probably just need to "hg qref". If they are different changes and you want to discard them, I think you do a "hg update".
(Assignee)

Comment 20

4 months ago
when i am trying to use "hg qnew bug-1383488-fix" its giving me the message that qnew is an unknown command
You probably need to enable the mercurial "mq" extension.
(Assignee)

Comment 22

4 months ago
Is it necessary to use mercurial queues for the patch or can it be done via mercurial also, because even after creating the .hgrc file and adding the mq extension I am not able to enable the extension.
You can use anything you want as long as you are comfortable with it, hell, I even use git to work on the gecko codebase :)
(Assignee)

Comment 24

3 months ago
I used "hg qnew bug-1383488-fix" to create the patch.Fixed the bug in one of the three files, used the cat and the export command.How can I get the link to submit it for review?
(Assignee)

Comment 25

3 months ago
Created attachment 8910629 [details] [diff] [review]
bug-1383488fix.patch

In this attachment I have fixed only one file, to check whether I have followed the process correctly.
Please do correct me if I am wrong.
Sorry for not getting back to you sooner. The format of that patch looks fine for your first bug (although it's difficult to be sure seeing it just removes a file - you also want to make sure you have 8 lines of context - ie, "diff -u8".) Also make sure the commit message says "Bug 1383488 - Trailing white spaces in sync code. r?eoger" when you put the next patch up.

While the process you are using probably isn't ideal, it will be good enough to get this first bug landed - so please submit your patch with a single .patch file which fixes all the files and put it up in this bug and we'll let you know if anything further needs to be done.

Thanks for persisting! :)
(Assignee)

Comment 27

3 months ago
Thanks for the review!!
I'll put all the files as soon as possible, and make the required changes.

Updated

3 months ago
Attachment #8910629 - Attachment is obsolete: true
(Assignee)

Comment 28

3 months ago
Created attachment 8911167 [details] [diff] [review]
Bug 138488 Trailing White Spaces in sync code.  r?eoger
Hi Arana,
Thank you for your first patch!
However, I think you mistaken the intent of that bug, our objective is to remove trailing spaces.

For example take this line:
http://searchfox.org/mozilla-central/source/services/sync/tps/extensions/mozmill/resource/stdlib/EventUtils.js#56
and select the text: you will see that there's a space after the last visible character.

If you check out comment 1, there's a search query that will allow you to find these.
(Assignee)

Comment 30

3 months ago
I will definitely correct the above mistake and resubmit the patch shortly.
(Assignee)

Comment 31

3 months ago
Created attachment 8913775 [details] [diff] [review]
bug-1383488.patch
Comment on attachment 8913775 [details] [diff] [review]
bug-1383488.patch

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

Thanks for the patch, but some of the files being changed here are wrong. What should be changed is:

services/sync/tps/extensions/mozmill/resource/stdlib/EventUtils.js
services/sync/tps/extensions/mozmill/resource/stdlib/httpd.js
services/sync/tps/extensions/mozmill/resource/stdlib/withs.js

however, what you actually changed is:

testing/mochitest/tests/SimpleTest/EventUtils.js
netwerk/test/httpserver/httpd.js
services/sync/tps/extensions/mozmill/resource/stdlib/withs.js

Note that only one of the above files is under services/sync.
(Assignee)

Comment 33

3 months ago
Created attachment 8914165 [details] [diff] [review]
Bug-1383488.patch
You need to log in before you can comment on or make changes to this bug.