Closed Bug 1365255 Opened 3 years ago Closed 2 years ago

DevTools: bring over documentation for contributing and coding standards into the tree

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: sole, Assigned: sole)

References

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → sole
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Comment on attachment 8871304 [details]
Bug 1365255 - DevTools: bring over documentation for contributing and coding standards into the tree.

https://reviewboard.mozilla.org/r/142790/#review146796

Thanks sole, looks great overall.
I added a couple of "add TODO for when we move to Github", I don't know if it's helpful or not.

One nit: We use both one or two spaces after dots in the documents. We should pick one or the other to be consistent (I prefer 1, but that's because it's how it is in french :D)

::: devtools/docs/contributing.md:7
(Diff revision 1)
> +
> +<!--TODO: make this less code focused and take ideas from debugger.html's contributing.md file-->
> +
> +Thank you for taking the time to contribute!
> +
> +Many people become contributors to implement a feature they miss, or want to fix something that annoys them. If you want to help, but don't know where to start, you could [look at our list of things to do](./bugs-issues.md). Alternatively, you might want to [report an issue or request a feature](./filing-good-bugs.md).

s/annoys/annoy I think ? Since "people" is plural.

::: devtools/docs/contributing/code-reviews.md:3
(Diff revision 1)
> +# Code reviews
> +
> +This checklist is primarily useful for DevTools reviewers as it lists all points potentially important to check while reviewing code, but it can serve as a set of guidelines for patch authors too.

s/all points potentially important/all of the potentially important points.

In fact I wonder if we could get rid of `potentially`.


Maybe we could split this sentence : 
"...to check while reviewing code. It can also serve..." 

What do you think ?

::: devtools/docs/contributing/code-reviews.md:8
(Diff revision 1)
> +This checklist is primarily useful for DevTools reviewers as it lists all points potentially important to check while reviewing code, but it can serve as a set of guidelines for patch authors too.
> +
> +## Bug status and patch file
> +
> +* Bug status is assigned, and assignee is correctly set
> +* Commit title and message follow the convention at: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

I guess we should take care of changing this when we move to github ? Maybe we could add a TODO here or something alike ?
Also we could make this an actual markdown link (or does markdown automatically convert URLs do links ?)

::: devtools/docs/contributing/code-reviews.md:17
(Diff revision 1)
> +
> +## Manual testing
> +
> +* Verify the feature or fix
> +* Report problems in the global review comment
> +* Decide which bugs need to block landing the change, and which can be fixed as follow-ups instead

This confuses me a little.
Maybe "bugs" could be changed to "chunk (of code)" (or something better), to avoid the confusion with a Bugzilla-bug.

Maybe we could start with saying: "if you find bugs on the patch, decide if they should block the chang or can be fixed in a follow-up patch(/issue)" ?

::: devtools/docs/contributing/code-reviews.md:21
(Diff revision 1)
> +* Report problems in the global review comment
> +* Decide which bugs need to block landing the change, and which can be fixed as follow-ups instead
> +
> +## Automated testing
> +
> +* Run new/modified tests, with and without e10s

We could link to something explaining :
- what's e10s
- how to run the test with and without it

::: devtools/docs/contributing/code-reviews.md:23
(Diff revision 1)
> +
> +## Automated testing
> +
> +* Run new/modified tests, with and without e10s
> +* Watch out for tests that pass but log exceptions or end before protocol requests are handled
> +* Watch out for slow/long tests: suggest many small tests rather than single long tests

we could even say to write unit tests when possible and integration tests only when it's really needed.

::: devtools/docs/contributing/code-reviews.md:30
(Diff revision 1)
> +## Code review
> +
> +* Code changes
> + * Review only what was changed by the contributor
> + * Code formatting follows [our ESLint rules](eslint.md) and [coding standards](./coding-standards.md)
> + * Code makes use of ES features when relevant

I don't understand this, can you explain it to me ?

::: devtools/docs/contributing/code-reviews.md:32
(Diff revision 1)
> +* Code changes
> + * Review only what was changed by the contributor
> + * Code formatting follows [our ESLint rules](eslint.md) and [coding standards](./coding-standards.md)
> + * Code makes use of ES features when relevant
> + * Code is properly commented, JSDoc is updated, new "public" methods all have JSDoc, see the [comment guidelines](./javascript.md#comments).
> + * If Promise code was added/modified, the right promise syntax is used, rejections are handled and Tasks are used when possible. See [asynchronous code](./javascript.md#asynchronous-code).

I don't know if we should advocate for Tasks now that async/await landed. In fact I think we have a bug filed to get rid of Tasks where we can, since it adds some overhead I use Promise.jsm under the hood if I remember correctly.

::: devtools/docs/contributing/code-reviews.md:35
(Diff revision 1)
> + * Code makes use of ES features when relevant
> + * Code is properly commented, JSDoc is updated, new "public" methods all have JSDoc, see the [comment guidelines](./javascript.md#comments).
> + * If Promise code was added/modified, the right promise syntax is used, rejections are handled and Tasks are used when possible. See [asynchronous code](./javascript.md#asynchronous-code).
> + * If a CSS file is added/modified, it follows [the CSS guidelines](./css.md)
> + * If a React or Redux module is added/modified it follows the [React/Redux guidelines](./javascript.md#react--redux)
> + * If devtools server code that should run in a worker is added/modified then it shouldn't use Services

s/devtools/DevTools

::: devtools/docs/contributing/code-reviews.md:39
(Diff revision 1)
> + * If a React or Redux module is added/modified it follows the [React/Redux guidelines](./javascript.md#react--redux)
> + * If devtools server code that should run in a worker is added/modified then it shouldn't use Services
> +* Test changes
> + * The feature or bug is [tested by new tests, or a modification of existing tests](../tests/writing-tests.md)
> + * [Test logging](../tests/writing-tests.md#logs-and-comments) is sufficient to help investigating test failures/timeouts
> + * [Test is e10s compliant](../tests/writing-tests.md#e10s-electrolysis) (no CPOWs, no content etc...).

Use the proper ellipsis character : "…" instead of 3 dots "..."

::: devtools/docs/contributing/code-reviews.md:41
(Diff revision 1)
> +* Test changes
> + * The feature or bug is [tested by new tests, or a modification of existing tests](../tests/writing-tests.md)
> + * [Test logging](../tests/writing-tests.md#logs-and-comments) is sufficient to help investigating test failures/timeouts
> + * [Test is e10s compliant](../tests/writing-tests.md#e10s-electrolysis) (no CPOWs, no content etc...).
> + * Tests are [clean and maintainable](../tests/writing-tests.md#writing-clean-maintainable-test-code)
> + * A try push has started (or even better, is green already)

Add a TODO to revisit this when we move to github ? Even if I don't really know how/if we are going to handle TRY then.

::: devtools/docs/contributing/code-reviews.md:50
(Diff revision 1)
> +## Finalize the review
> +* R+: the code should land as soon as possible
> +* R+ with comments: there are some comments, but they are minor enough, or don't require a new review once addressed, trust the author
> +* R cancel / R- / F+: there is something wrong with the code, and a new review is required

Same here, add a TODO to change this when we are on Github

::: devtools/docs/contributing/coding-standards.md:3
(Diff revision 1)
> +# Coding standards
> +
> +Our code base is quite large, and a lot of different people contribute to it all the time, so it's important to share standards to keep the code consistent and written in a predictable style that also helps avoid common mistakes.

We could split this sentence into 2 since it's pretty long: 
"Our code base is quite large, and a lot of different people contribute to it all the time. So it's important..."

::: devtools/docs/contributing/css.md:3
(Diff revision 1)
> +# CSS
> +
> +This page is for information about CSS used by the DevTools toolbox.  Wondering about the Dev Edition theme?  See this page for more information about the [Developer Edition theme](https://wiki.mozilla.org/DevTools/Developer_Edition_Theme).

DevTools Toolbox seems redundant, we could use "DevTools" only maybe ?

::: devtools/docs/contributing/css.md:7
(Diff revision 1)
> +
> +This page is for information about CSS used by the DevTools toolbox.  Wondering about the Dev Edition theme?  See this page for more information about the [Developer Edition theme](https://wiki.mozilla.org/DevTools/Developer_Edition_Theme).
> +
> +## Basics
> +
> +You can find existing CSS at https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes

Add a TODO to make sure we change this when we move to Github

::: devtools/docs/contributing/css.md:24
(Diff revision 1)
> +
> +Make sure each file starts with the standard copyright header (see [License Boilerplate](https://www.mozilla.org/MPL/headers/)).
> +
> +### Testing
> +
> +CSS changes to the toolbox should generally be similar across platforms since they used a shared implementation, but there can still be differences worth checking out. Check major changes on Windows, OSX and Ubuntu.

could we remove "to the toolbox" ?

::: devtools/docs/contributing/css.md:33
(Diff revision 1)
> +selector,
> +alternate-selector {
> +  property: value;
> +  other-property: other-value;
> +}

could we add examples for long shorthand properties, and multi-valued properties (background, font-family, ...0

::: devtools/docs/contributing/css.md:86
(Diff revision 1)
> +In order to support retina displays and Windows DPI settings, it's recommended to use SVG since it keeps the CSS clean. However, if only 1x and 2x PNG assets are available, you can use this @media query to target HDPI: `@media (min-resolution: 1.1dppx)`
> +
> +## Performance
> +
> +* Read [Writing Efficient CSS](https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS).
> +* Use an iframe where possible so your rules are scoped to the smallest possible set of nodes

Is this still true, and could this refined (say when it's appropriate to use an iframe and when it's not)

::: devtools/docs/contributing/css.md:117
(Diff revision 1)
> +* Going to about:config and setting `intl.uidirection.en` to rtl is not recommended, and will always require to re-open devtools to have any impact.
> +
> +## Toggles
> +
> +Sometimes you have a style that you want to turn on and off. For example a tree twisty, a tab background, etc.
> +<!--TODO: what is a 'tree twisty???'-->The Mozilla way is to perform the toggle using an attribute rather than a class:

twisty is the expand-collapse arrow, I was confused by the term too :)

::: devtools/docs/contributing/css.md:120
(Diff revision 1)
> +.tree-node {
> +  background-image: url(right-arrow.png);
> +}
> +.tree-node[open] {
> +  background-image: url(down-arrow.png);
> +}

we say to use SVG everywhere, maybe we could make those rules use one instead of a png :D

::: devtools/docs/contributing/eslint.md:3
(Diff revision 1)
> +# Using ESLint in DevTools
> +
> +The main rule set is in `devtools/.eslintrc`. It is meant to be used with ESLint 3.5.0.

Add a todo to change the path when we're in Github

::: devtools/docs/contributing/eslint.md:11
(Diff revision 1)
> +
> +## Installing
> +
> +From the root of the project type:
> +
> +`./mach eslint --setup`

Add a TODO to change this when we move to Github

::: devtools/docs/contributing/eslint.md:19
(Diff revision 1)
> +The preferred way of running ESLint from the command line is by using `mach` like this:
> +
> +```bash
> +./mach eslint path/to/directory
> +```

Add a TODO to change this when we move to Github

::: devtools/docs/contributing/eslint.md:25
(Diff revision 1)
> +
> +```bash
> +./mach eslint path/to/directory
> +```
> +
> +This makes sure that ESLint runs with the same configuration that on our CI environment (see the next section).

s/makes sure/ensures ?

::: devtools/docs/contributing/eslint.md:33
(Diff revision 1)
> +If you are pushing a patch to the [`try` repository](https://wiki.mozilla.org/ReleaseEngineering/TryServer) to run the tests, then you can also tell it to run the ESLint job and therefore verify that you did not introduce new errors.
> +
> +If you build on all platforms, then the ESLint job will run by default, but if you selected a few platforms only in your  [trysyntax](https://wiki.mozilla.org/Build:TryChooser), then you need to also add `eslint-gecko` as a target platform for ESLint to run.

Add a TODO to change this when we move to Github

::: devtools/docs/contributing/eslint.md:148
(Diff revision 1)
> +
> +![ESLint in VS Code](eslint-vscode.png)
> +
> +### Fixing ESLint Errors
> +
> +Here is a list of advice for people writing new code that, hopefully, will help with writing eslint-clean code:

s/advice/advices ? Not sure of this one

::: devtools/docs/contributing/javascript.md:5
(Diff revision 1)
> +# JavaScript coding standards
> +
> +Probably the best piece of advice is **to be consistent with the rest of the code in the file**.
> +
> +We use [ESLint](http://eslint.org/) to analyse JavaScript files automatically, either from within a code editor or from the command line. Here's [our guide to install and configure it](./eslint.md).

s/analyse/analyze

::: devtools/docs/contributing/javascript.md:25
(Diff revision 1)
> +* aim for short functions, 24 lines max (ESLint has a rule that checks for function complexity too),
> +* `aArguments aAre the aDevil` (don't use them please),
> +* `"use strict";` globally per module,
> +* `semicolons; // use them`,
> +* no comma-first,
> +* consider using Task.jsm (`Task.async`, `Task.spawn`) for nice-looking asynchronous code instead of formatting endless `.then` chains,<!--TODO: shouldn't we advise async/await now?-->

I think your comment is right, we should advocate for async/await
Attachment #8871304 - Flags: review?(nchevobbe) → review+
Comment on attachment 8871304 [details]
Bug 1365255 - DevTools: bring over documentation for contributing and coding standards into the tree.

https://reviewboard.mozilla.org/r/142790/#review146796

> s/annoys/annoy I think ? Since "people" is plural.

the subject of this sentence is "something", not people ;-)

> s/all points potentially important/all of the potentially important points.
> 
> In fact I wonder if we could get rid of `potentially`.
> 
> 
> Maybe we could split this sentence : 
> "...to check while reviewing code. It can also serve..." 
> 
> What do you think ?

Good point! I've rephrased it a lot now, and split it in two generous lines:

This checklist is primarily aimed at reviewers, as it lists important points to check while reviewing a patch.

It can also be useful for patch authors: if the changes comply with these guidelines, then it's more likely the review will be approved.

> I guess we should take care of changing this when we move to github ? Maybe we could add a TODO here or something alike ?
> Also we could make this an actual markdown link (or does markdown automatically convert URLs do links ?)

TODO introduced, things made into links (also FYI, Markdown does convert URLs to links but they are long and ugly so I turned them into links)

> This confuses me a little.
> Maybe "bugs" could be changed to "chunk (of code)" (or something better), to avoid the confusion with a Bugzilla-bug.
> 
> Maybe we could start with saying: "if you find bugs on the patch, decide if they should block the chang or can be fixed in a follow-up patch(/issue)" ?

Yes, that was a bit confusing. I replaced it with mentions to "problems":

Decide if any of those problems should block landing the change, or if they can be filed as follow-up bugs instead, to be fixed later

> We could link to something explaining :
> - what's e10s
> - how to run the test with and without it

Linked to our page on tests, e10s section

> we could even say to write unit tests when possible and integration tests only when it's really needed.

Added:

* Watch out for new tests written as integration tests instead of as unit tests: unit tests should be the preferred option, when possible.

> I don't understand this, can you explain it to me ?

I've actually removed that sentence because it is already better explained on the coding standards.

> I don't know if we should advocate for Tasks now that async/await landed. In fact I think we have a bug filed to get rid of Tasks where we can, since it adds some overhead I use Promise.jsm under the hood if I remember correctly.

Yes! There are TODOs elsewhere to do this so I have removed the mention to Task and left the pointer to asynchronous code.

> We could split this sentence into 2 since it's pretty long: 
> "Our code base is quite large, and a lot of different people contribute to it all the time. So it's important..."

I have splitted it into THREE!

> Add a TODO to make sure we change this when we move to Github

Replaced it with:

The CSS code is in `devtools/client/themes`.

... which saves us one follow up

> could we remove "to the toolbox" ?

Yes! The person who wrote the document initially loved to talk about the toolbox.

> could we add examples for long shorthand properties, and multi-valued properties (background, font-family, ...0

Out of scope for this commit, I will add a TODO.

> Is this still true, and could this refined (say when it's appropriate to use an iframe and when it's not)

Out of scope for this commit, I added a TODO.

> twisty is the expand-collapse arrow, I was confused by the term too :)

Ah! thanks, I have added a (note)

> we say to use SVG everywhere, maybe we could make those rules use one instead of a png :D

spot on! I replaced them :)

> Add a todo to change the path when we're in Github

It would still be in devtools/ if that's the name of the repo (very likely), I'll leave it as it is for now

> Add a TODO to change this when we move to Github

Added a TODO for the whole file

> s/makes sure/ensures ?

they're synonymous but I replaced it anyway so it's only one word :P

> s/advice/advices ? Not sure of this one

I simplified it to:

"This should help you write eslint-clean code"

> s/analyse/analyze

it's fine in UK English!
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4f4dee0ff7b2
DevTools: bring over documentation for contributing and coding standards into the tree. r=nchevobbe
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4f4dee0ff7b2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.