Closed Bug 1383488 Opened 7 years ago Closed 6 years ago

Trailing whitespace in Sync source code

Categories

(Firefox :: Sync, enhancement, P3)

Product:
Firefox
Component:
Sync
Version:
56 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: nicolaso, Unassigned, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(3 files, 1 obsolete file)

Bug 138488 Trailing White Spaces in sync code. r?eoger
436.91 KB, patch
Details | Diff | Splinter Review
bug-1383488.patch
9.00 KB, patch
Details | Diff | Splinter Review
Bug-1383488.patch
17.07 KB, patch
Details | Diff | Splinter Review 
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.
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 :)
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).
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.
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.
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.
Could you correct me if I am wrong, the other two files are pointed by lines 1185 and 4972
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!
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
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)
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.
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".
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.
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 :)
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?
Attached patch bug-1383488fix.patch (obsolete) — — Splinter Review 
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! :)
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
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.
I will definitely correct the above mistake and resubmit the patch shortly.

    
    

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

    
    

    
  
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
Closed: 6 years ago
Resolution: --- → INVALID

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: