Trailing whitespace in Sync source code

RESOLVED INVALID

Status

()

P3
normal
RESOLVED INVALID
2 years ago
11 months ago

People

(Reporter: nicolaso, Unassigned, Mentored)

Tracking

({good-first-bug})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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

2 years ago
Component: Untriaged → Sync
Mentor: eoger
Keywords: good-first-bug
Priority: -- → P3
Status: UNCONFIRMED → NEW
Ever confirmed: true
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 :)

Comment 2

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

Comment 4

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

Comment 6

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

Comment 8

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

Comment 10

2 years ago
Could you correct me if I am wrong, the other two files are pointed by lines 1185 and 4972

Comment 11

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

Comment 13

2 years 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.
Assignee: nobody → aruna.maurya12

Comment 14

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

Comment 16

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

Comment 18

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

Comment 20

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

Comment 22

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

Comment 24

a year 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?

Comment 25

a year 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! :)

Comment 27

a year ago
Thanks for the review!!
I'll put all the files as soon as possible, and make the required changes.
Attachment #8910629 - Attachment is obsolete: true

Comment 28

a year 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.

Comment 30

a year ago
I will definitely correct the above mistake and resubmit the patch shortly.

Comment 31

a year 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.

Comment 33

a year ago
Created attachment 8914165 [details] [diff] [review]
Bug-1383488.patch
Assignee: aruna.maurya12 → nobody

Comment 34

11 months ago
I had submitted a patch 6 months ago, but no one reviewed. This bug can still be fixed, if someone just reviews the patch I submitted long back, instead of removing me from being the assignee.
I'm really sorry, Aruna, I thought you'd lost interest in the bug, since Mark's comments from comment 32 still apply to your latest patch: you removed whitespace from `services/sync/tps/extensions/mozmill/resource/stdlib/withs.js`, but not the others; and `testing/mochitest/*` and `netwerk/test/*` shouldn't have been changed.

However, MozMill is now gone (bug 1415957), so there's nothing to do here.
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → INVALID

Comment 36

11 months ago
Very well then. Wasn't aware of the fact that MozMill has been removed.
You need to log in before you can comment on or make changes to this bug.