Add Contributing documentation #368

Merged
merged 3 commits into from Dec 19, 2016

Projects

None yet

3 participants

@AutomatedTester
Member
AutomatedTester commented Nov 29, 2016 edited

When Pull requests and issues are opened, Github automatically links to
the contributing file so that people are aware of it. This hopefully
means that contributors will follow some of the rules.


This change is Reviewable

@AutomatedTester AutomatedTester Add Contributing documentation
When Pull requests and issues are opened, Github automatically links to
the contributing file so that people are aware of it. This hopefully
means that contributors will follow some of the rules.
f3548fd
@jgraham
Collaborator
jgraham commented Nov 29, 2016

Review status: 0 of 1 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 15 at r1 (raw file):
For many of the problems we get it's at least as useful to see the logs so it's clear what's being sent over the wire. Those are also easier to produce than a good minimal testcase. I think we should rewrite this section to be more geckodriver-specific e.g.:

For issue reports to be actionable, it must be clear exactly what the observed and expected behaviours are, and how to set up the state required to observe the erroneous behaviour. The most useful thing to provide is a minimal HTML file which allows the problem to be reproduced, plus a debug-level log from geckodriver showing the wire-protocol calls used to set up the problem. The HTML file should ideally have only the code required to reproduce the problem with as little extraneous material as possible (it should not, for example, be a copy of your production site unless every element on the page is required to reproduce the bug). Because of the wide variety of client bindings for WebDriver, clients scripts and logs are typically not very useful if the verbose geckodriver logs are available. Issues relating to a specific client should be filed in the issue tracker of that project.


CONTRIBUTING.md, line 46 at r1 (raw file):

We practice HEAD-based development, which means all changes are applied

I can't tell what that actually means to judge if it's true or not.


CONTRIBUTING.md, line 62 at r1 (raw file):

any).  Follow these guidelines when writing one:

1. The first line should be around 50 characters or less and contain a

That is not a "rule" that I'm interested in enforcing.


CONTRIBUTING.md, line 65 at r1 (raw file):

   short description of the change.
2. Keep the second line blank.
3. Wrap all other lines at 72 columns.

Or that, really. Anything close to 80 columns seems fine. In general I don't think we should have a wall of rules to put off contributors, and I certainly wouldn't reject a patch for having 73 columns in the commit message (I might if it had like 200, so I think a guideline of "around 80" is fine).


CONTRIBUTING.md, line 69 at r1 (raw file):

   fixes, if any.

A good commit message can look like this:

I feel like this is a lot of information about trivia vs the size of the document. Can we remove this example?


CONTRIBUTING.md, line 109 at r1 (raw file):

Pull requests are usually reviewed within a few days. If there are
comments to address, apply your changes in new commits (preferably
[fixups](http://git-scm.com/docs/git-commit)) and push to the same

I don't think we really care about fixups. Given that we use reviewable, amends are also OK. Mentioning that review will typically be conducted using reviewable seems like a more useful thing to mention here.


CONTRIBUTING.md, line 115 at r1 (raw file):

When code review is complete, a committer will take your PR and
integrate it on Selenium's master branch. Because we like to keep a

This is not Selenium.


CONTRIBUTING.md, line 122 at r1 (raw file):

GeckoDriver contributors frequent the `#ateam` channel on
[`irc.freenode.org`](https://webchat.freenode.net/).

That would be irc.mozilla.org.


Comments from Reviewable

@AutomatedTester
Member

Review status: 0 of 1 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 15 at r1 (raw file):

Previously, jgraham wrote…

For many of the problems we get it's at least as useful to see the logs so it's clear what's being sent over the wire. Those are also easier to produce than a good minimal testcase. I think we should rewrite this section to be more geckodriver-specific e.g.:

For issue reports to be actionable, it must be clear exactly what the observed and expected behaviours are, and how to set up the state required to observe the erroneous behaviour. The most useful thing to provide is a minimal HTML file which allows the problem to be reproduced, plus a debug-level log from geckodriver showing the wire-protocol calls used to set up the problem. The HTML file should ideally have only the code required to reproduce the problem with as little extraneous material as possible (it should not, for example, be a copy of your production site unless every element on the page is required to reproduce the bug). Because of the wide variety of client bindings for WebDriver, clients scripts and logs are typically not very useful if the verbose geckodriver logs are available. Issues relating to a specific client should be filed in the issue tracker of that project.

fixed


CONTRIBUTING.md, line 46 at r1 (raw file):

Previously, jgraham wrote…

I can't tell what that actually means to judge if it's true or not.

It's true :)


CONTRIBUTING.md, line 62 at r1 (raw file):

Previously, jgraham wrote…

That is not a "rule" that I'm interested in enforcing.

removed this section


CONTRIBUTING.md, line 65 at r1 (raw file):

Previously, jgraham wrote…

Or that, really. Anything close to 80 columns seems fine. In general I don't think we should have a wall of rules to put off contributors, and I certainly wouldn't reject a patch for having 73 columns in the commit message (I might if it had like 200, so I think a guideline of "around 80" is fine).

I'll remove this section and we can guide people if they have rubbish commits


CONTRIBUTING.md, line 69 at r1 (raw file):

Previously, jgraham wrote…

I feel like this is a lot of information about trivia vs the size of the document. Can we remove this example?

removed this section


CONTRIBUTING.md, line 109 at r1 (raw file):

Previously, jgraham wrote…

I don't think we really care about fixups. Given that we use reviewable, amends are also OK. Mentioning that review will typically be conducted using reviewable seems like a more useful thing to mention here.

Done.


CONTRIBUTING.md, line 115 at r1 (raw file):

Previously, jgraham wrote…

This is not Selenium.

Done.


CONTRIBUTING.md, line 122 at r1 (raw file):

Previously, jgraham wrote…

That would be irc.mozilla.org.

Done.


Comments from Reviewable

@AutomatedTester AutomatedTester fixup! Add Contributing documentation
56fdfc3
@jgraham
Collaborator
jgraham commented Nov 30, 2016

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 46 at r1 (raw file):

Previously, AutomatedTester (David Burns) wrote…

It's true :)

OK, but since I don't know what it means I think it's probably OK to remove from this document.


CONTRIBUTING.md, line 10 at r2 (raw file):

GeckoDriver

I think we spell this as geckodriver in other places (change here and throughout).


CONTRIBUTING.md, line 19 at r2 (raw file):

up the problem. Please provide [concise reproducible test
cases](http://sscce.org/) and describe what results you are seeing and what
results you expect.. Because of the wide variety of client bindings for

Two full stops.


Comments from Reviewable

@jgraham
Collaborator
jgraham commented Nov 30, 2016

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

CONTRIBUTING.md
@@ -0,0 +1,99 @@
+# Contributing to GeckoDriver
@andreastt
andreastt Dec 8, 2016 Member

s/GeckoDriver/geckodriver/g

CONTRIBUTING.md
+The GeckoDriver project welcomes contributions from everyone. There are a
+number of ways you can help:
+
+## Bug Reports
@andreastt
andreastt Dec 8, 2016 Member

s/Bug/Issue/

CONTRIBUTING.md
+observed and expected behaviours are, and how to set up the state required
+to observe the erroneous behaviour. The most useful thing to provide is a
+minimal HTML file which allows the problem to be reproduced, plus a
+debug-level log from geckodriver showing the wire-protocol calls used to set
@andreastt
andreastt Dec 8, 2016 Member

s/debug/trace/

CONTRIBUTING.md
+debug-level log from geckodriver showing the wire-protocol calls used to set
+up the problem. Please provide [concise reproducible test
+cases](http://sscce.org/) and describe what results you are seeing and what
+results you expect.. Because of the wide variety of client bindings for
@andreastt
andreastt Dec 8, 2016 Member

Two full stops.

CONTRIBUTING.md
+
+## Code Contributions
+
+The GeckoDriver project welcomes new contributors.
@andreastt
andreastt Dec 8, 2016 Member

We already say this above.

CONTRIBUTING.md
+
+If you're looking for easy bugs, have a look at
+[issues labelled E-easy](https://github.com/mozilla/geckodriver/issues?utf8=%E2%9C%93&q=is%3Aopen+is%3Aissue+label%3Aeasy+)
+on Github.
@andreastt
andreastt Dec 8, 2016 Member

s/Github/GitHub/g apparently.

CONTRIBUTING.md
+### Step 5: Push
+
+```text
+% git push origin my-feature-branch
@andreastt
andreastt Dec 8, 2016 Member

origin is superfluous here.

CONTRIBUTING.md
+% git push origin my-feature-branch
+```
+
+Go to https://github.com/yourusername/geckodriver.git and press the _Pull
@andreastt
andreastt Dec 8, 2016 Member

s/.git//

CONTRIBUTING.md
+Request_ and fill out the form.
+
+Pull requests are usually reviewed within a few days. Reviews will be done
+through [Reviewable](https://reviewable.io/reviews/mozilla/geckodriver)
@andreastt
andreastt Dec 8, 2016 Member

FWIW I’m unable to log in to Reviewable right now and I frequently do reviews on GitHub. Perhaps just leave it out?

CONTRIBUTING.md
+## Communication
+
+GeckoDriver contributors frequent the `#ateam` channel on
+[`irc.mozilla.org`](http://chat.mibbit.com/?server=irc.mozilla.org:#ateam).
@andreastt
andreastt Dec 8, 2016 Member

Point out that this is an IRC channel.

@AutomatedTester AutomatedTester fixup! fixup! Add Contributing documentation
add309e
@AutomatedTester
Member

Review status: 0 of 1 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 1 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

s/GeckoDriver/geckodriver/g

Done.


CONTRIBUTING.md, line 6 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

s/Bug/Issue/

Done.


CONTRIBUTING.md, line 10 at r2 (raw file):

Previously, jgraham wrote…

GeckoDriver

I think we spell this as geckodriver in other places (change here and throughout).

Done.


CONTRIBUTING.md, line 16 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

s/debug/trace/

Done.


CONTRIBUTING.md, line 19 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Two full stops.

Done.


CONTRIBUTING.md, line 19 at r2 (raw file):

Previously, jgraham wrote…

Two full stops.

Done.


CONTRIBUTING.md, line 26 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

We already say this above.

Done.


CONTRIBUTING.md, line 30 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

s/Github/GitHub/g apparently.

Done.


CONTRIBUTING.md, line 80 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

origin is superfluous here.

I am going to leave it here. A number of auto-complete tools only work if the remote name is used before the branch


CONTRIBUTING.md, line 83 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

s/.git//

Done.


CONTRIBUTING.md, line 87 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

FWIW I’m unable to log in to Reviewable right now and I frequently do reviews on GitHub. Perhaps just leave it out?

I am going to leave it.


CONTRIBUTING.md, line 99 at r2 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Point out that this is an IRC channel.

Adding the channel in the url just cuts out the step to pick a channel and the channel name is already mentioned in the channel


Comments from Reviewable

@andreastt
Member

Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


CONTRIBUTING.md, line 80 at r2 (raw file):

Previously, AutomatedTester (David Burns) wrote…

I am going to leave it here. A number of auto-complete tools only work if the remote name is used before the branch

OK


CONTRIBUTING.md, line 87 at r2 (raw file):

Previously, AutomatedTester (David Burns) wrote…

I am going to leave it.

OK


CONTRIBUTING.md, line 99 at r2 (raw file):

Previously, AutomatedTester (David Burns) wrote…

Adding the channel in the url just cuts out the step to pick a channel and the channel name is already mentioned in the channel

Right, yes. LGTM.


Comments from Reviewable

@andreastt
Member

@AutomatedTester Since jgraham is on PTO, feel free to land this at your convenience. I believe most if not all the issues have been clarified.

@AutomatedTester AutomatedTester merged commit bbdca67 into mozilla:master Dec 19, 2016

1 of 2 checks passed

code-review/reviewable 1 file, 3 discussions left (andreastt, AutomatedTester, jgraham)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment