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

Status: RESOLVED FIXED

(DevTools :: about:debugging, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

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

[migueluseche]

I would prefer someone new to take this, but if nobody takes it for the next week I would like to do it.

Comment 2

[Maxime Vaillancourt]

Hi there — this would be my first contribution to Firefox. Can I try my hand at it?

Comment 3

[Julian Descottes [:jdescottes]]

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!

Comment 4

[Maxime Vaillancourt]

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

Comment 5

[Maxime Vaillancourt]

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?

Comment 6

[Maxime Vaillancourt]

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

Comment 7

[Julian Descottes [:jdescottes]]

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

Comment 8

[Maxime Vaillancourt]

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

Thanks a lot for your help Julian - it's much appreciated as a newcomer. Let me know if this updated patch works.

Comment 9

[Julian Descottes [:jdescottes]]

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!

Comment 10

[Maxime Vaillancourt]

Thank you for providing such a great first contribution experience!

Comment 11

[Julian Descottes [:jdescottes]]

Tests are green, adding checkin-needed.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8526281637d
CamelCase all aboutdebugging component files. r=jdescottes

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a8526281637d

Comment 14

[alexander]

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.

Comment 15

[Julian Descottes [:jdescottes]]

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

[alexander]

(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

[migueluseche]

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

[alexander]

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.