Closed Bug 1402391 Opened 7 years ago Closed 7 years ago

CamelCase all React component files in \devtools\client\responsive.html\src\components\

Categories

(DevTools :: Responsive Design Mode, enhancement, P3)

enhancement

Tracking

(firefox57 fix-optional, firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: pbro, Assigned: migueluseche, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

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

However the files in \devtools\client\responsive.html\src\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, just running the built Firefox and opening the RDM tool should be enough of a test that things still work.
Blocks: 1402359
No longer blocks: 1402387
Can I take this? I wanna start collaborating
Sure, thank you for wanting to help.
A few more precisions:
- the moz.build file in this directory will also need to be updated (it lists the files)
- we use a module loading system in DevTools, so you will also need to update the require path in some of the files you will change. For example in file viewport-toolbar.js there is a line like this:
const DeviceSelector = createFactory(require("./device-selector"));
Since you will rename the file device-selector.js to DeviceSelector.js, you will need to change this line to:
const DeviceSelector = createFactory(require("./DeviceSelector"));

Once you've done all the changes, please make sure to build your local Firefox version based on these changes, then run it and verify that RDM still works.

I'll assign the bug to you now, let me know if you need more help.
Assignee: nobody → migueluseche
Status: NEW → ASSIGNED
(In reply to migueluseche from comment #1)
> Can I take this? I wanna start collaborating
Hey, just checking up on this. Are you OK with comment 2 to get started? Do you need more help?
Flags: needinfo?(migueluseche)
Thanks for the explanation. I can do this next weekend.
Flags: needinfo?(migueluseche)
Attached patch bug-1402391.patch (obsolete) — Splinter Review
I made the rename for that folder. Then I compiled again and I ran all tests and all of them passed.
Attachment #8914348 - Flags: feedback?
Comment on attachment 8914348 [details] [diff] [review]
bug-1402391.patch

Thank you for your work on this Miguel.
I'll forward the feedback request to Julian.
Attachment #8914348 - Flags: feedback? → feedback?(jdescottes)
Comment on attachment 8914348 [details] [diff] [review]
bug-1402391.patch

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

Thanks for the patch! 

However I can't apply it on my machine. I believe it's because my file system is case insensitive and it tries adding devtools/client/responsive.html/components/Viewport.js before removing devtools/client/responsive.html/components/viewport.js (or any other similar rename).

To move files, you should use the mercurial command mv rather than renaming the files by hand.
So here:

  hg devtools/client/responsive.html/components/viewport.js devtools/client/responsive.html/components/Viewport.js

etc... for all the other files.

This way we will not lose the changeset history for the files and we will be able to apply your patch easily on any filesystem.
Attachment #8914348 - Flags: feedback?(jdescottes)
I see, I used hg rename to preserver history.  I can revert it to be hg move. 
May I ask you what should I do to discard current commit? Do i need to go back to a previous commit and start over? Or is there something to undo last commit? I couldn't find that info in the docs.
(In reply to migueluseche from comment #8)
> I see, I used hg rename to preserver history.  I can revert it to be hg
> move. 
> May I ask you what should I do to discard current commit? Do i need to go
> back to a previous commit and start over? Or is there something to undo last
> commit? I couldn't find that info in the docs.

Weird, `hg mv` should be an alias for `hg rename`, but your patch did not record the changes as files being renamed. I quickly tested locally, and here is the patch I get for a single file rename:

  # HG changeset patch
  # User Julian Descottes <jdescottes@mozilla.com>
  # Date 1507046306 -7200
  #      Tue Oct 03 17:58:26 2017 +0200
  # Node ID 35bd5c90b9959096d591ad0ed4365ff9b9337638
  # Parent  d245ec39bd4cdb84c482f2443cea785a9884b243
  test

  diff --git a/devtools/client/responsive.html/components/viewport.js b/devtools/client/responsive.html/components/Viewport.js
  rename from devtools/client/responsive.html/components/viewport.js
  rename to devtools/client/responsive.html/components/Viewport.js

Your patch file should mostly contain similar items + the references you updated in the files using the component.

To discard your current commit you have several options:
- hg strip {commit-id} (for instance `hg strip .` to discard your current commit)
  => this will completely remove the commit, losing all the changes
- or do some changes and then `hg commit --amend` to add the changes to the current commit

In this case since we are talking about moving files, I guess it would be easier to simply use strip and start from scratch again.
Hello, I retried with hg move but hives the same result, I read something like this: https://stackoverflow.com/questions/10531853/resolving-mercurial-case-folding-collision-in-windows Is it possible to review it on a Linux/Unix machine? or it will affect any Windows user?
Also here's a possible solution https://www.mercurial-scm.org/wiki/FixingCaseCollisions
(In reply to migueluseche from comment #10)
> Hello, I retried with hg move but hives the same result, I read something
> like this:
> https://stackoverflow.com/questions/10531853/resolving-mercurial-case-
> folding-collision-in-windows Is it possible to review it on a Linux/Unix
> machine? or it will affect any Windows user?
> Also here's a possible solution
> https://www.mercurial-scm.org/wiki/FixingCaseCollisions

I am testing it on OSX. Beyond the fact that the patch will fail to apply for all users that have a case insensitive file system, this patch also destroys the history for the files (just tested on Ubuntu). So no we can't go with this. 

Which version of mercurial are you using?
I'm using Mercurial version 4.3.2. I'm gonna try to make the patch from another way. I think the problem is the patch generation and not the commit itself (I'm using same instruction as mozilla docs hg diff > my_patch.diff )
hg diff should also keep the move info correctly.

Could you share the commands you are using and the response from the command line?

For example here is what I get when doing a sample move.

> kglazko-23706:fx-team jdescottes$ hg --version
> Mercurial Distributed SCM (version 4.3.1)
> (see https://mercurial-scm.org for more information)
> 
> Copyright (C) 2005-2017 Matt Mackall and others
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> kglazko-23706:fx-team jdescottes$ hg status
> kglazko-23706:fx-team jdescottes$ hg rename devtools/shared/fronts/memory.js devtools/shared/fronts/memory2.js
> kglazko-23706:fx-team jdescottes$ hg diff 
> diff --git a/devtools/shared/fronts/memory.js b/devtools/shared/fronts/memory2.js
> rename from devtools/shared/fronts/memory.js
> rename to devtools/shared/fronts/memory2.js
> kglazko-23706:fx-team jdescottes$ 

I moved a random file and as you can see my diff is only 3 lines long. 

(In reply to migueluseche from comment #12)
> itself (I'm using same instruction as mozilla docs hg diff > my_patch.diff )

Another thing is that using `diff` would not create a patch such as the one you attached. This looks like the result of an export (which is fine, that's how I export all my patches). 

Maybe it has to do with the way you commit? 
Following my previous snippets, commit and export looks like:

> kglazko-23706:fx-team jdescottes$ hg commit -m 'test commit'
> created new head
> kglazko-23706:fx-team jdescottes$ hg export -r .
> # HG changeset patch
> # User Julian Descottes <jdescottes@mozilla.com>
> # Date 1507190983 -7200
> #      Thu Oct 05 10:09:43 2017 +0200
> # Node ID 5130a5bf730ae029709cf92d69d5e2e15fc6e1ba
> # Parent  9be05b2177667ed8221f9da4fdcc200dbdf3de62
> test commit
> 
> MozReview-Commit-ID: FY6pGfg9uVi
> 
> diff --git a/devtools/shared/fronts/memory.js b/devtools/shared/fronts/memory2.js
> rename from devtools/shared/fronts/memory.js
> rename to devtools/shared/fronts/memory2.js
> kglazko-23706:fx-team jdescottes$ 

It will be easier to make progress based on the output of your command line. Feel free to ping me on IRC or on slack (https://devtools-html-slack.herokuapp.com/) if you would like to chat about that rather than do it on this bug.
Attached patch bug-1402391-git.patch (obsolete) — Splinter Review
Hi, I think I found the issue. I need to export the commit in git format. That way the patch renames the file instead of deleting and creating. This patch is like bug 1402389.

I exported it with: hg export --git > bug-1402391-git.patch
Attachment #8914348 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Comment on attachment 8916851 [details] [diff] [review]
bug-1402391-git.patch

Changing need information flag from mentor to jdescottes.
Flags: needinfo?(jdescottes)
(In reply to migueluseche from comment #14)
> Created attachment 8916851 [details] [diff] [review]
> bug-1402391-git.patch
> 
> Hi, I think I found the issue. I need to export the commit in git format.
> That way the patch renames the file instead of deleting and creating. This
> patch is like bug 1402389.
> 
> I exported it with: hg export --git > bug-1402391-git.patch

Great find! You can set update your .hgrc file with the following setting in the diff section.

  [diff]
  git = 1

This way you won't have to specify git every time (I think :) )

The new patch looks good, I'll review shortly
Flags: needinfo?(jdescottes)
Comment on attachment 8916851 [details] [diff] [review]
bug-1402391-git.patch

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

Patch looks good to me, thanks for doing this! 

I submitted your changeset to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8764e288a9e9ce39ede0e31050baed1d6ce94771
This is our continuous integration platform and this will run the devtools tests with your patch. The results will be displayed on the page linked above.

If the tests are green, we will move forward with the patch and land it.

Just a small thing, can you update your commit message to change from r=pbro to r=jdescottes ?
Thanks!
Attachment #8916851 - Flags: review+
Flags: needinfo?(pbrosset)
Attachment #8916851 - Attachment is obsolete: true
Attachment #8916979 - Flags: review+
Attachment #8916979 - Attachment is patch: true
Tests are green, I just updated the commit message to change the reviewer. I am adding the checkin-needed keyword on this bug. A sheriff will now pick your patch and land it on one of our integration branches.

After that the patch will be released together with Firefox 58. 

Thanks a lot for your contribution Miguel (and for getting through the hg export pains :) )
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/672c8297a9b4
CamelCase all React component files in \devtools\client\responsive.html\src\components\. r=jdescottes
Keywords: checkin-needed
Nice to hear that! You're welcome. Gonna move to bug 1402390
https://hg.mozilla.org/mozilla-central/rev/672c8297a9b4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: