CamelCase all React component files in \devtools\client\aboutdebugging\components

RESOLVED FIXED in Firefox 58

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: jdescottes, Assigned: maxvaillancourt1, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 58

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
We recently agreed to name our React component file names using the CamelCase convention.

However the files in \devtools\client\aboutdebugging\components\ do not follow this convention, so let's rename them.

This is an easy fix, but make sure you read the contribution docs before starting: http://docs.firefox-dev.tools/ so that you can build Firefox locally and verify that things still work after your change.

After building, to test that things still work, run Firefox, open about:debugging and try the different categories.

https://developer.mozilla.org/en-US/docs/Tools/about:debugging

Comment 1

2 years ago
I would prefer someone new to take this, but if nobody takes it for the next week I would like to do it.
(Assignee)

Comment 2

2 years ago
Hi there — this would be my first contribution to Firefox. Can I try my hand at it?
(Reporter)

Comment 3

2 years ago
Thanks Maxime, assigning the bug to you! 

Go though the contribution doc to get your development environment setup. If you need any help, find us on Slack (https://devtools-html-slack.herokuapp.com/) or IRC: #devtools at irc.mozilla.org.

Some additional info for this bug: 
- each JS file to rename is indexed in a moz.build file found in the same folder as the file.
  When renaming panel-menu-entry.js to PanelMenuEntry.js, the moz.build file needs to be updated accordingly
- we load files using a require() method. Calls will need to be updated as well: require("./panel-menu-entry") will become require("./PanelMenuEntry")

=> A good way to know if you have everything covered is to use searchfox, our web based code search: http://searchfox.org/
For "panel-menu-entry", you can look up all the current references to this string:
http://searchfox.org/mozilla-central/search?q=panel-menu-entry&case=false&regexp=false&path=devtools%2Fclient

Let me know if you have any question!
Assignee: nobody → maxvaillancourt1
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
Looks like my patch does not include the actual renaming of the files. I followed the contribution guide (http://docs.firefox-dev.tools/contributing/making-prs.html) to create the patch. Did I miss anything to include filename changes?
(Assignee)

Comment 6

2 years ago
Just realized that the patch does not include the updated "require()" calls either. How can I include those in the patch, as well as the actual filename changes? Thanks for your help! :)
(Reporter)

Comment 7

2 years ago
(In reply to Maxime Vaillancourt from comment #5)
> Looks like my patch does not include the actual renaming of the files. I
> followed the contribution guide
> (http://docs.firefox-dev.tools/contributing/making-prs.html) to create the
> patch. Did I miss anything to include filename changes?

Depends how you renamed the files. Let's run `hg status` and paste the result here.  

If you renamed the files manually in your filesystem, they probably are no longer tracked by mercurial. 
And your hg status contains a lot of 
! path/to/old-name.js
! path/to/NewName.js

You would need to add them so that mercurial can track them too (`hg add .`). But don't do that.

If you are using hg, the correct way is to use `hg rename` (or `hg mv`, it is an alias).
This way the history of the files will be preserved.

So for example, to move aboutdebugging.js to Aboutdebugging.js

> jdescottes$ hg mv devtools/client/aboutdebugging/components/aboutdebugging.js devtools/client/aboutdebugging/components/Aboutdebugging.js
> jdescottes$ hg status
> A devtools/client/aboutdebugging/components/Aboutdebugging.js
> R devtools/client/aboutdebugging/components/aboutdebugging.js
> jdescottes$ hg diff
> diff --git a/devtools/client/aboutdebugging/components/aboutdebugging.js b/devtools/client/aboutdebugging/components/Aboutdebugging.js
> rename from devtools/client/aboutdebugging/components/aboutdebugging.js
> rename to devtools/client/aboutdebugging/components/Aboutdebugging.js

(the `hg status` & `hg diff` here help making sure that the file was renamed in a way compatible with mercurial, but you don't have to run them for each file you move!)

But first, if I am correct and you have a bunch of manually renamed files, you need to cleanup your workspace. I think the easiest would be to use the `purge` extension (https://www.mercurial-scm.org/wiki/PurgeExtension). Then run `hg revert -C . && hg purge`. After that your `hg status` should be clean and you can start moving files using `hg mv`.

Once you have moved all the files, you can use `hg commit --amend` to modify your existing changeset. Let me know if you are stuck. 

(In reply to Maxime Vaillancourt from comment #6)
> Just realized that the patch does not include the updated "require()" calls
> either. How can I include those in the patch, as well as the actual filename
> changes? Thanks for your help! :)

That's probably just linked to the issue above.
(Assignee)

Comment 8

2 years ago
Thanks a lot for your help Julian - it's much appreciated as a newcomer. Let me know if this updated patch works.
Attachment #8921721 - Attachment is obsolete: true
Attachment #8921721 - Flags: feedback?(jdescottes)
Attachment #8921863 - Flags: feedback?(jdescottes)
(Reporter)

Updated

2 years ago
Attachment #8921863 - Attachment is patch: true
(Reporter)

Comment 9

2 years ago
Comment on attachment 8921863 [details] [diff] [review]
CamelCase all React component files in \devtools\client\aboutdebugging\components

This new patch looks good Maxime, thanks! about:debugging seems to work fine, tests are still passing locally, so I'm setting r+.

I just pushed it to our continuous integration server. The devtools test suite will run with your patch, to make sure we didn't introduce any regression.

Results will be displayed on this page : https://treeherder.mozilla.org/#/jobs?repo=try&revision=142999c7678ecb28766c62f734753602c2c76920

If the tests look good, I'll mark the bug as checkin-needed. A sheriff will then pick it up and land it on one of our integration branches. Thanks a lot for helping out here!
Attachment #8921863 - Flags: feedback?(jdescottes) → review+
(Assignee)

Comment 10

2 years ago
Thank you for providing such a great first contribution experience!
(Reporter)

Comment 11

2 years ago
Tests are green, adding checkin-needed.
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8526281637d
CamelCase all aboutdebugging component files. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a8526281637d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Comment 14

a year ago
Just to add 2 cents here: when you renaming file only by changing caps - you make potential problem to people, who works with Firefox project on Windows platform - there are no difference between caps and lower letter in filename on Windows. Please, count this moment in future.
(Reporter)

Comment 15

a year ago
My filesystem is case insensitive. We have several developers working on Windows machines primarily. 
What kind of issue did you get with this renaming?

Comment 16

a year ago
(In reply to Julian Descottes [:jdescottes][:julian] from comment #15)
> My filesystem is case insensitive. We have several developers working on
> Windows machines primarily. 
> What kind of issue did you get with this renaming?

Because we did a changes in this file in our product (based on FF) - on merge with new FF version, on Windows, this file marked as deleted, and it is not possible to simply add it back (need "to dance around" to put it back to repo). Of course I can handle this, but I think in general this is not a good practice to do so (just my personal opinion)

Comment 17

a year ago
Do you have mercurial's git extension activated? I had problems doing this with a clean mercurial installation, but using git patches I had no problem because it detects a renaming instead deleting and recreating the file.

Comment 18

a year ago
We are using only git, FF source code we copy to our own repo into separate branch as "snapshots", time-to-time. We don't need all commits history.

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.