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

Matt Traudt's avatar
Matt Traudt committed
3
Contributing to Simple Bandwidth Scanner
juga's avatar
juga committed
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:

juga's avatar
juga committed
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
---------------------------------
juga's avatar
juga committed
15

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

.. _ticket-ref:

juga's avatar
juga committed
20
* Open a issue in
21
  `Tor Project Gitlab <https://gitlab.torproject.org/tpo/network-health/sbws/-/issues>`_ .
juga's avatar
juga committed
22

juga's avatar
juga committed
23
24
25
26
Code/documentation patches
---------------------------

The sbws canonical repository is https://gitweb.torproject.org/sbws.git,
27
28
29
but we review patches using the Gitlab repository
(https://gitlab.torproject.org/tpo/network-health/sbws/-/merge_requests)
Merge Requests (MR).
juga's avatar
juga committed
30

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

33
34
35
36
37
38
39
40
41
42
.. 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.

43
Steps to create a MR
44
45
~~~~~~~~~~~~~~~~~~~~~

46
1. Create a issue in Tor Project Gitlab (:ref:`Open issue <ticket-ref>`)
47
48
2. Fork ``sbws`` via the Gitlab web interface:
   https://gitlab.torproject.org/tpo/network-health/sbws
juga's avatar
juga committed
49
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
7. Write code (:ref:`codestyle-ref`), tests, documentation,
   extra files (:ref:`extrafiles-ref`), commit (:ref:`commits-ref`), etc.
8. Ensure tests pass (./TESTING.rst).
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
9. Push your branch to your Gitlab repository.

We are temporally using Github Travis to ensure tests pass. For this:

10. Clone ``sbws`` via the Github web interface:
    https://github.com/torproject/sbws
11. Push your branch to your Github repository.

12. If you have an account in Travis, you can see whether it pass the tests in
    Github and at https://travis-ci.org/youruser/sbws/

Finally:

13. Create a MR from your branch at
    https://gitlab.torproject.org/tpo/network-health/sbws


77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101

.. _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
102
External link: `Code Style <https://docs.python-guide.org/writing/style/>`_
Matt Traudt's avatar
Matt Traudt committed
103

104
105
106
107
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
108

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

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

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

juga's avatar
juga committed
116
117
118
119
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
120
Functional style is prefered:
Matt Traudt's avatar
Matt Traudt committed
121

juga's avatar
juga committed
122
123
124
125
- 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
126

juga's avatar
juga committed
127
[FUNC]_
Matt Traudt's avatar
Matt Traudt committed
128

juga's avatar
juga committed
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
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
146
~~~~~~~~~
Matt Traudt's avatar
Matt Traudt committed
147

148
Each commit should reference the Tor Project Gitlab issue (example: ``#12345``)
149
and possibly the bugfix version.
150
The commit message should contain ``Closes: #bugnumber``.
151

152
153
154
155
156
157
158
159
160
161
162
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.
163

juga's avatar
juga committed
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
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
182

183
When a MR is being reviewed, new changes might be needed:
Matt Traudt's avatar
Matt Traudt committed
184

juga's avatar
juga committed
185
- If the change does not modify a previous change, create new commits and push.
juga's avatar
juga committed
186
- If the change modifies a previous change and it's small,
187
  `git commit fixup <https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---fixupltcommitgt>`_
188
  should be used. When it is agreed that the MR is ready, create a new branch
juga's avatar
juga committed
189
190
191
  named ``mybranch_02`` and run:

  .. code-block:: bash
192

juga's avatar
juga committed
193
    rebase --autosquash
Matt Traudt's avatar
Matt Traudt committed
194

195
196
  push, create new MR and close old MR mentioning the number of the new MR.
- If the review takes long and when it's ready code related to the MR has changed
juga's avatar
juga committed
197
198
199
  in master, create a new branch named ``mybranch_02`` and run:

  .. code-block:: bash
juga's avatar
juga committed
200
201
202

    rebase master

203
  push, create new MR and close old MR mentioning the number of the new MR.
juga's avatar
juga committed
204

juga's avatar
juga committed
205
[MERG]_
juga's avatar
juga committed
206

juga's avatar
juga committed
207
.. _review-ref:
juga's avatar
juga committed
208

juga's avatar
juga committed
209
210
Reviewing code
----------------
juga's avatar
juga committed
211

juga's avatar
juga committed
212
213
214
215
216
217
218
219
220
221
222
223
224
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,
225
226
  before merging or closing a MR.
- Should merge (not rebase) the MR.
juga's avatar
juga committed
227
228
- 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
229
  create a new MR from it, as explained above.
juga's avatar
juga committed
230
231
- 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,
232
  push the changes and merge that MR.
juga's avatar
juga committed
233
  The contributor should be notified about it.
234
- If the reviewer realize that new changes are needed after the MR has been
juga's avatar
juga committed
235
236
237
  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
238
  MR if there was not any feedback after a week.
juga's avatar
juga committed
239
240
- Should not push directly to master, unless changes are trivial (typos,
  extra spaces, etc.)
241
- Should not push to master new features while there are open MRs to review.
juga's avatar
juga committed
242

juga's avatar
juga committed
243
244
245
Currently, the reviewers are `gk <https://gitlab.torproject.org/gk>`_,
`ahf <https://gitlab.torproject.org/ahf>`_,
`juga <https://gitlab.torproject.org/juga>`_.
juga's avatar
juga committed
246
247
248
249

.. _releases-ref:

Releases
juga's avatar
juga committed
250
251
----------

juga's avatar
juga committed
252
253
254
Releases follow `semantic versioning`_.
Until release 1.0.0 is reached, this project is not considered production
ready.
juga's avatar
juga committed
255

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

juga's avatar
juga committed
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
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::
276

juga's avatar
juga committed
277
278
279
280
    Implement something to compare error with current consensus.
- A dirauth should be able to understand the documentation, otherwise the
  documentation should be clarified.

281
282
283
.. _changelog:

Create a ./CHANGELOG.rst file.
284
Each entry should reference the Tor Project Gitlab issue (example: ``#12345``)
285
286
and possibly the bugfix version.
Until version 1.0.2 we have followed `keep a changelog`_ format.
287
288
289
290

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
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324

.. _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
325
.. _`keep a changelog`: https://keepachangelog.com/en/1.0.0/
juga's avatar
juga committed
326
327
.. _`semantic versioning`: https://semver.org/
.. _`vulture`: https://pypi.org/project/vulture/
328
.. _`gitchangelog`: https://github.com/vaab/gitchangelog