GitLab is used only for code review, issue tracking and project management. Canonical locations for source code are still https://gitweb.torproject.org/ https://git.torproject.org/ and git-rw.torproject.org.

contributing.rst 10.7 KB
Newer Older
juga  's avatar
juga committed
1 2
.. _contributing:

Matt Traudt's avatar
Matt Traudt committed
3
Contributing to Simple Bandwidth Scanner
4
=========================================
Matt Traudt's avatar
Matt Traudt committed
5

juga  's avatar
juga committed
6
Thank you for your interest in Simple Bandwidth Scanner (``sbws``).
Matt Traudt's avatar
Matt Traudt committed
7

juga  's avatar
juga committed
8 9
Examples of contributions include:

10
* Bug reports, feature requests
juga  's avatar
juga committed
11
* Code/documentation patches
Matt Traudt's avatar
Matt Traudt committed
12

juga  's avatar
juga committed
13 14
Bug reports or feature requests
---------------------------------
15

juga  's avatar
juga committed
16 17 18 19 20
* Check that it has not been already reported.

.. _ticket-ref:

* Open a ticket in
21
  `Tor Project Trac <https://trac.torproject.org>`_
juga  's avatar
juga committed
22
  and assign the component to ``Core Tor``/``sbws``.
23

juga  's avatar
juga committed
24 25 26 27 28 29 30
Code/documentation patches
---------------------------

The sbws canonical repository is https://gitweb.torproject.org/sbws.git,
but we review patches using the Github canonical repository
(https://github.com/torproject/sbws) Pull Requests (PR).

31
To know more about ``sbws`` code,
juga  's avatar
juga committed
32

33 34 35 36 37 38 39 40 41 42 43 44 45 46
.. seealso::

  - :ref:`dev_doc`
  - ``./docs/source/testing.rst`` (or `testing </docs/source/testing.rst>`_
    or :ref:`testing`).
  - ``./docs/source/documenting.rst`` (or `documenting </docs/source/documenting.rst>`_
    or :ref:`documenting`).

The following are guidelines we aim to follow.

Steps to create a PR
~~~~~~~~~~~~~~~~~~~~~

1. Create a ticket in Tor Project Trac (:ref:`Open ticket <ticket-ref>`)
juga  's avatar
juga committed
47 48 49
2. Clone ``sbws`` via the Github web interface
   https://github.com/torproject/sbws
3. Clone the repository locally
50
4. Install ``sbws`` as explained in ./INSTALL.rst and ./TESTING.rst
juga  's avatar
juga committed
51 52 53 54 55 56
   Use ``pip install -e <>``
5. If needed install the documentation and build it as explained in
   ./DOCUMENTATION.rst
6. Create a new branch, named ``ticketXXX``.
   Optionally, name it with a string explaining what it does,
   ie ``ticketXXX_contributing``
57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89
7. Write code (:ref:`codestyle-ref`), tests, documentation,
   extra files (:ref:`extrafiles-ref`), commit (:ref:`commits-ref`), etc.
8. Ensure tests pass (./TESTING.rst).
9. Push your branch to your repository. If you have an account in Travis,
   you can see whether it pass the tests in Github and in
   https://travis-ci.org/youruser/sbws/
10. Create a PR from your branch to https://github.com/torproject/sbws
11. Change the Trac ticket status to ``needs_review``

.. _codestyle-ref:

Code style
~~~~~~~~~~

Follow the Zen of Python (:pep:`20`)

.. code-block:: pycon

    >>> import this
    The Zen of Python, by Tim Peters

    Beautiful is better than ugly.
    Explicit is better than implicit.
    Simple is better than complex.
    Complex is better than complicated.
    Flat is better than nested.
    Sparse is better than dense.
    Readability counts.

Code should adhere to the :pep:`8` guidelines.
Before release 1.0.0, some guidelines have not been followed,
such as the ordering the inputs (:pep:`8#imports`).

juga  's avatar
juga committed
90
External link: `Code Style <https://docs.python-guide.org/writing/style/>`_
Matt Traudt's avatar
Matt Traudt committed
91

92 93 94 95
All functions, methods and classes should have :pep:`0257`
(except ``__repr__`` and ``__str__``).
Before release 1.0.0, some docstrigs do not have 3 double quotes ``"""``
(:pep:`0257#id15`).
Matt Traudt's avatar
Matt Traudt committed
96

juga  's avatar
juga committed
97
External link: `Documentation <https://docs.python-guide.org/writing/documentation/>`_
Matt Traudt's avatar
Matt Traudt committed
98

juga  's avatar
juga committed
99
New features should add a corresponding documentation in /docs.
juga  's avatar
juga committed
100

101 102 103
An editor compatible with `EditorConfig <https://editorconfig.org/>`_ will
help you to follow the general formatting code style.

juga  's avatar
juga committed
104 105 106 107
Timestamps must be in UTC. It is prefered to use ``datetime`` objects or
Unix timestamps. Timestamps read by the user should be always formatted in
`ISO 8601 <https://en.wikipedia.org/wiki/ISO_8601>`_

juga  's avatar
juga committed
108
Functional style is prefered:
Matt Traudt's avatar
Matt Traudt committed
109

juga  's avatar
juga committed
110 111 112 113
- use list comprenhensions lambda, map, reduce
- avoid reasigigning variables, instead create new ones
- use ``deepcopy`` when passing list of objects to a function/method
- classes should change attributes only in one method (other than __init__?)
Matt Traudt's avatar
Matt Traudt committed
114

juga  's avatar
juga committed
115
[FUNC]_
Matt Traudt's avatar
Matt Traudt committed
116

juga  's avatar
juga committed
117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133
In general, do not reinvent the wheel, use Python native modules as ``logging``,
instead of implementing similar functionality.
Or use other packages when the new dependency can be extra, for instance
`vulture`_.

.. _`extrafiles-ref`:

Extra required files
~~~~~~~~~~~~~~~~~~~~~

Any non-trivial change should contain tests. See ./TESTING.rst.
When running tests, currently ``flake8`` informs on some PEP8 errors/warnings,
but not all.

.. _commits-ref:

Commits
juga  's avatar
juga committed
134
~~~~~~~~~
Matt Traudt's avatar
Matt Traudt committed
135

136 137
Each commit should reference the Tor Project Trac ticket (example: ``#12345``)
and possibly the bugfix version.
138
The commit message should contain ``Closes: #bugnumber``.
139

140 141 142 143 144 145 146 147 148 149 150
From version 1.0.2 we started to prefix the summary with the subpackage or
component, though we have not standarized the words to use, eg: ``scanner``,
``generate``, ``v3bwfile``, ``relaylist``, ``doc``, ``test``, ``CI``.

From version 1.0.3, we also started to prefix the summary with ``new``,
``fix`` or ``chg``, so that `gitchangelog`_ automatically generates different
sections in the CHANGELOG.

From version 1.1.0 we started to use the words ``new``, ``chg`` and ``fix``,
not in the sense `gitchangelog`_ use them, but to match sematic versioning
changes major, minor and patch.
151

juga  's avatar
juga committed
152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169
Try to make each commit a logically separate changes.::

  As a general rule, your messages should start with a single line that’s
  o more than about 50 characters and that describes the changeset concisely,
  followed by a blank line, followed by a more detailed explanation.
  The Git project requires that the more detailed explanation include
  your motivation for the change and contrast its implementation with
  previous behavior — this is a good guideline to follow.
  It’s also a good idea to use the imperative present tense in these messages.
  In other words, use commands.
  Instead of "I added tests for" or "Adding tests for," use "Add tests for."

[DIST]_

Template originally written by `Tim Pope`_: :ref:`example commit <commit-msg>`

Code being reviewed workflow
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Matt Traudt's avatar
Matt Traudt committed
170

juga  's avatar
juga committed
171
When a PR is being reviewed, new changes might be needed:
Matt Traudt's avatar
Matt Traudt committed
172

juga  's avatar
juga committed
173
- If the change does not modify a previous change, create new commits and push.
juga  's avatar
juga committed
174
- If the change modifies a previous change and it's small,
175
  `git commit fixup <https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---fixupltcommitgt>`_
juga  's avatar
juga committed
176
  should be used. When it is agreed that the PR is ready, create a new branch
juga  's avatar
juga committed
177 178 179
  named ``mybranch_02`` and run:

  .. code-block:: bash
180

juga  's avatar
juga committed
181
    rebase --autosquash
Matt Traudt's avatar
Matt Traudt committed
182

juga  's avatar
juga committed
183 184
  push, create new PR and close old PR mentioning the number of the new PR.
- If the review takes long and when it's ready code related to the PR has changed
juga  's avatar
juga committed
185 186 187
  in master, create a new branch named ``mybranch_02`` and run:

  .. code-block:: bash
juga  's avatar
juga committed
188 189 190 191 192

    rebase master

  push, create new PR and close old PR mentioning the number of the new PR.

juga  's avatar
juga committed
193
[MERG]_
juga  's avatar
juga committed
194

juga  's avatar
juga committed
195
.. _review-ref:
juga  's avatar
juga committed
196

juga  's avatar
juga committed
197 198
Reviewing code
----------------
juga  's avatar
juga committed
199

juga  's avatar
juga committed
200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223 224 225 226 227 228 229 230 231 232 233 234 235 236
All code should be peer-reviewed. Two reasons for this are::

    Because a developer cannot think of everything at once;
    Because a fresh pair of eyes may spot an error, a corner-case in the code,
    insufficient documentation, a missing consistency check, etc.

[REVI]_

Reviewers:

- Should let the contributor know what to improve/change.
- Should not push code to the contributor's branch.
- Should wait for contributor's changes or feedback after changes are requested,
  before merging or closing a PR.
- Should merge (not rebase) the PR.
- If rebase is needed due to changes in master, the contributor should create
  a new branch named `xxx_rebased` based on the reviewed branch, rebase and
  create a new PR from it, as explained above.
- If new changes are needed when the contributor's branch is ready to merge,
  the reviewer can create a new branch based on the contributor's branch,
  push the changes and merge that PR.
  The contributor should be notified about it.
- If the reviewer realize that new changes are needed after the PR has been
  merged, the reviewer can push to master, notifying the contributor about the
  changes.
- Because currently there are not many reviewers, reviewers can merge their own
  PR if there was not any feedback after a week.
- Should not push directly to master, unless changes are trivial (typos,
  extra spaces, etc.)
- Should not push to master new features while there are open PRs to review.

Currently, the reviewers are the persons that have contributed to the code:
pastly, teor, juga.

.. _releases-ref:

Releases
juga  's avatar
juga committed
237 238
----------

juga  's avatar
juga committed
239 240 241
Releases follow `semantic versioning`_.
Until release 1.0.0 is reached, this project is not considered production
ready.
juga  's avatar
juga committed
242

juga  's avatar
juga committed
243
Currently development happens in master, this might change from release 1.0.0
Matt Traudt's avatar
Matt Traudt committed
244

juga  's avatar
juga committed
245 246 247 248 249 250 251 252 253 254 255 256 257 258 259 260 261 262
so that master has the last release changes, and development happens in the
next release branch.

Before major releases, ensure that:

- Installation from scratch, as specified in ./INSTALL.md, must success.
- All tests must pass.
- Tor must be able to parse the produced bw files
  (current way is manual)

  .. todo::

    Test that run Tor as dirauth and parse the files

- Bandwidth files must produce graphs compatible with Torflow
  (current way to test it is manual)

  .. todo::
263

juga  's avatar
juga committed
264 265 266 267
    Implement something to compare error with current consensus.
- A dirauth should be able to understand the documentation, otherwise the
  documentation should be clarified.

268 269 270 271 272 273
.. _changelog:

Create a ./CHANGELOG.rst file.
Each entry should reference the Tor Project Trac ticket (example: ``#12345``)
and possibly the bugfix version.
Until version 1.0.2 we have followed `keep a changelog`_ format.
274 275 276 277

From version 1.1.x, run ``./scripts/maint/release.py`` to create new releases.
It uses `gitchangelog`_ to automatically add new CHANGELOG entries from the
commits' messages.
juga  's avatar
juga committed
278 279 280 281 282 283 284 285 286 287 288 289 290 291 292 293 294 295 296 297 298 299 300 301 302 303 304 305 306 307 308 309 310 311

.. _commit-msg:

Example commit message
-----------------------

::

  Short (50 chars or less) summary of changes

  More detailed explanatory text, if necessary.  Wrap it to
  about 72 characters or so.  In some contexts, the first
  line is treated as the subject of an email and the rest of
  the text as the body.  The blank line separating the
  summary from the body is critical (unless you omit the body
  entirely); tools like rebase can get confused if you run
  the two together.

  Further paragraphs come after blank lines.

    - Bullet points are okay, too

    - Typically a hyphen or asterisk is used for the bullet,
      preceded by a single space, with blank lines in
      between, but conventions vary here


.. rubric:: External eferences

.. [DIST] https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project
.. [MERG] https://www.atlassian.com/git/tutorials/merging-vs-rebasing
.. [REVI] https://doc.sagemath.org/html/en/developer/reviewer_checklist.html
.. [FUNC] https://medium.com/@rohanrony/functional-programming-in-python-1-lambda-map-filter-reduce-zip-8739ea144186
.. _tim pope: https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
312
.. _`keep a changelog`: https://keepachangelog.com/en/1.0.0/
juga  's avatar
juga committed
313 314
.. _`semantic versioning`: https://semver.org/
.. _`vulture`: https://pypi.org/project/vulture/
315
.. _`gitchangelog`: https://github.com/vaab/gitchangelog