contributing.rst 14 KB

123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221222223224225226227228229230231232233234235236237238239240241242243244245246247248249250251252253254255256257258259260261262263264265266267268269270271272273274275276277278279280281282283284285286287288289290291292293294295296297298299300301302303304305306307308309310311312313314315316317318319320321322323324325326327328329330331332333334335336337338339340341342343344345346347348349350351352353354355356357358359360361362363364365366367368369370371372373374375376377378379380381382383384385386387388389390391392393394395396397398399400401402403404405406407408409410411412413414415416417418419420421422423424425426427428429430431432433434435436437438439440441442443444445446447448449450451452453454
  1. ============
  2. Contributing
  3. ============
  4. .. contents::
  5. Contributing happens via "Pull requests" (PR) on github. Every PR needs to be
  6. reviewed before it can be merged and the Continuous Integration should be green.
  7. The PR has to be approved (and is often merged too) by one "code owner", either
  8. by the code owner who is responsible for the subsystem the PR belongs to or by
  9. two core developers or by Araq.
  10. See `codeowners <codeowners.html>`_ for more details.
  11. Writing tests
  12. =============
  13. There are 3 types of tests:
  14. 1. ``runnableExamples`` documentation comment tests, ran by ``nim doc mymod.nim``
  15. These end up in documentation and ensure documentation stays in sync with code.
  16. 2. tests in ``when isMainModule:`` block, ran by ``nim c mymod.nim``
  17. ``nimble test`` also typially runs these in external nimble packages.
  18. 3. testament tests, e.g.: ``tests/stdlib/tos.nim`` (only used for Nim repo).
  19. Not all the tests follow the convention here, feel free to change the ones
  20. that don't. Always leave the code cleaner than you found it.
  21. Stdlib
  22. ------
  23. If you change the stdlib (anything under ``lib/``, e.g. ``lib/pure/os.nim``),
  24. put a test in the file you changed. Add the tests under a ``when isMainModule:``
  25. condition so they only get executed when the tester is building the
  26. file. Each test should be in a separate ``block:`` statement, such that
  27. each has its own scope. Use boolean conditions and ``doAssert`` for the
  28. testing by itself, don't rely on echo statements or similar.
  29. Sample test:
  30. .. code-block:: nim
  31. when isMainModule:
  32. block: # newSeqWith tests
  33. var seq2D = newSeqWith(4, newSeq[bool](2))
  34. seq2D[0][0] = true
  35. seq2D[1][0] = true
  36. seq2D[0][1] = true
  37. doAssert seq2D == @[@[true, true], @[true, false],
  38. @[false, false], @[false, false]]
  39. # doAssert with `not` can now be done as follows:
  40. doAssert not (1 == 2)
  41. Newer tests tend to be run via ``testament`` rather than via ``when isMainModule:``,
  42. e.g. ``tests/stdlib/tos.nim``; this allows additional features such as custom
  43. compiler flags; for more details see below.
  44. Compiler
  45. --------
  46. The tests for the compiler use a testing tool called ``testament``. They are all
  47. located in ``tests/`` (e.g.: ``tests/destructor/tdestructor3.nim``).
  48. Each test has its own file. All test files are prefixed with ``t``. If you want
  49. to create a file for import into another test only, use the prefix ``m``.
  50. At the beginning of every test is the expected behavior of the test.
  51. Possible keys are:
  52. - ``cmd``: A compilation command template e.g. ``nim $target --threads:on $options $file``
  53. - ``output``: The expected output (stdout + stderr), most likely via ``echo``
  54. - ``exitcode``: Exit code of the test (via ``exit(number)``)
  55. - ``errormsg``: The expected compiler error message
  56. - ``file``: The file the errormsg was produced at
  57. - ``line``: The line the errormsg was produced at
  58. For a full spec, see here: ``testament/specs.nim``
  59. An example for a test:
  60. .. code-block:: nim
  61. discard """
  62. errormsg: "type mismatch: got (PTest)"
  63. """
  64. type
  65. PTest = ref object
  66. proc test(x: PTest, y: int) = nil
  67. var buf: PTest
  68. buf.test()
  69. Running tests
  70. =============
  71. You can run the tests with
  72. ::
  73. ./koch tests
  74. which will run a good subset of tests. Some tests may fail. If you
  75. only want to see the output of failing tests, go for
  76. ::
  77. ./koch tests --failing all
  78. You can also run only a single category of tests. A category is a subdirectory
  79. in the ``tests`` directory. There are a couple of special categories; for a
  80. list of these, see ``testament/categories.nim``, at the bottom.
  81. ::
  82. ./koch tests c lib # compiles/runs stdlib modules, including `isMainModule` tests
  83. ./koch tests c megatest # runs a set of tests that can be combined into 1
  84. To run a single test:
  85. ::
  86. ./koch test run <category>/<name> # e.g.: tuples/ttuples_issues
  87. ./koch test run tests/stdlib/tos.nim # can also provide relative path
  88. For reproducible tests (to reproduce an environment more similar to the one
  89. run by Continuous Integration on travis/appveyor), you may want to disable your
  90. local configuration (e.g. in ``~/.config/nim/nim.cfg``) which may affect some
  91. tests; this can also be achieved by using
  92. ``export XDG_CONFIG_HOME=pathtoAlternateConfig`` before running ``./koch``
  93. commands.
  94. Comparing tests
  95. ===============
  96. Because some tests fail in the current ``devel`` branch, not every failure
  97. after your change is necessarily caused by your changes. Some tests are
  98. flaky and will fail on occasion; these are typically bugs that should be fixed.
  99. Test failures can be grepped using ``Failure:``.
  100. The tester can compare two test runs. First, you need to create the
  101. reference test. You'll also need to the commit id, because that's what
  102. the tester needs to know in order to compare the two.
  103. ::
  104. git checkout devel
  105. DEVEL_COMMIT=$(git rev-parse HEAD)
  106. ./koch tests
  107. Then switch over to your changes and run the tester again.
  108. ::
  109. git checkout your-changes
  110. ./koch tests
  111. Then you can ask the tester to create a ``testresults.html`` which will
  112. tell you if any new tests passed/failed.
  113. ::
  114. ./koch tests --print html $DEVEL_COMMIT
  115. Deprecation
  116. ===========
  117. Backward compatibility is important, so instead of a rename you need to deprecate
  118. the old name and introduce a new name:
  119. .. code-block:: nim
  120. # for routines (proc/template/macro/iterator) and types:
  121. proc oldProc(a: int, b: float): bool {.deprecated:
  122. "deprecated since v1.2.3; use `newImpl: string -> int` instead".} = discard
  123. # for (const/var/let/fields) the msg is not yet supported:
  124. const Foo {.deprecated.} = 1
  125. # for enum types, you can deprecate the type or some elements
  126. # (likewise with object types and their fields):
  127. type Bar {.deprecated.} = enum bar0, bar1
  128. type Barz = enum baz0, baz1 {.deprecated.}, baz2
  129. See also `Deprecated <https://nim-lang.org/docs/manual.html#pragmas-deprecated-pragma>`_
  130. pragma in the manual.
  131. Documentation
  132. =============
  133. When contributing new procs, be sure to add documentation, especially if
  134. the proc is public. Even private procs benefit from documentation and can be
  135. viewed using ``nim doc --docInternal foo.nim``.
  136. Documentation begins on the line
  137. following the ``proc`` definition, and is prefixed by ``##`` on each line.
  138. Runnable code examples are also encouraged, to show typical behavior with a few
  139. test cases (typically 1 to 3 ``assert`` statements, depending on complexity).
  140. These ``runnableExamples`` are automatically run by ``nim doc mymodule.nim``
  141. as well as ``testament`` and guarantee they stay in sync.
  142. .. code-block:: nim
  143. proc addBar*(a: string): string =
  144. ## Adds "Bar" to `a`.
  145. runnableExamples:
  146. assert "baz".addBar == "bazBar"
  147. result = a & "Bar"
  148. See `parentDir <https://nim-lang.github.io/Nim/os.html#parentDir%2Cstring>`_
  149. example.
  150. The RestructuredText Nim uses has a special syntax for including code snippets
  151. embedded in documentation; these are not run by ``nim doc`` and therefore are
  152. not guaranteed to stay in sync, so ``runnableExamples`` is usually preferred:
  153. .. code-block:: nim
  154. proc someproc*(): string =
  155. ## Return "something"
  156. ##
  157. ## .. code-block::
  158. ## echo someproc() # "something"
  159. result = "something" # single-hash comments do not produce documentation
  160. The ``.. code-block:: nim`` followed by a newline and an indentation instructs the
  161. ``nim doc`` command to produce syntax-highlighted example code with the
  162. documentation (``.. code-block::`` is sufficient from inside a nim module).
  163. When forward declaration is used, the documentation should be included with the
  164. first appearance of the proc.
  165. .. code-block:: nim
  166. proc hello*(): string
  167. ## Put documentation here
  168. proc nothing() = discard
  169. proc hello*(): string =
  170. ## ignore this
  171. echo "hello"
  172. The preferred documentation style is to begin with a capital letter and use
  173. the imperative (command) form. That is, between:
  174. .. code-block:: nim
  175. proc hello*(): string =
  176. ## Return "hello"
  177. result = "hello"
  178. or
  179. .. code-block:: nim
  180. proc hello*(): string =
  181. ## says hello
  182. result = "hello"
  183. the first is preferred.
  184. Best practices
  185. =============
  186. Note: these are general guidelines, not hard rules; there are always exceptions.
  187. Code reviews can just point to a specific section here to save time and
  188. propagate best practices.
  189. .. _noimplicitbool:
  190. Take advantage of no implicit bool conversion
  191. .. code-block:: nim
  192. doAssert isValid() == true
  193. doAssert isValid() # preferred
  194. .. _design_for_mcs:
  195. Design with method call syntax chaining in mind
  196. .. code-block:: nim
  197. proc foo(cond: bool, lines: seq[string]) # bad
  198. proc foo(lines: seq[string], cond: bool) # preferred
  199. # can be called as: `getLines().foo(false)`
  200. .. _avoid_quit:
  201. Use exceptions (including assert / doAssert) instead of ``quit``
  202. rationale: https://forum.nim-lang.org/t/4089
  203. .. code-block:: nim
  204. quit() # bad in almost all cases
  205. doAssert() # preferred
  206. .. _tests_use_doAssert:
  207. Use ``doAssert`` (or ``require``, etc), not ``assert`` in all tests so they'll
  208. be enabled even in release mode (except for tests in ``runnableExamples`` blocks
  209. which for which ``nim doc`` ignores ``-d:release``).
  210. .. code-block:: nim
  211. when isMainModule:
  212. assert foo() # bad
  213. doAssert foo() # preferred
  214. .. _delegate_printing:
  215. Delegate printing to caller: return ``string`` instead of calling ``echo``
  216. rationale: it's more flexible (e.g. allows caller to call custom printing,
  217. including prepending location info, writing to log files, etc).
  218. .. code-block:: nim
  219. proc foo() = echo "bar" # bad
  220. proc foo(): string = "bar" # preferred (usually)
  221. .. _use_Option:
  222. [Ongoing debate] Consider using Option instead of return bool + var argument,
  223. unless stack allocation is needed (e.g. for efficiency).
  224. .. code-block:: nim
  225. proc foo(a: var Bar): bool
  226. proc foo(): Option[Bar]
  227. .. _use_doAssert_not_echo:
  228. Tests (including in testament) should always prefer assertions over ``echo``,
  229. except when that's not possible. It's more precise, easier for readers and
  230. maintaners to where expected values refer to. See for example
  231. https://github.com/nim-lang/Nim/pull/9335 and https://forum.nim-lang.org/t/4089
  232. .. code-block:: nim
  233. echo foo() # adds a line for testament in `output:` block inside `discard`.
  234. doAssert foo() == [1, 2] # preferred, except when not possible to do so.
  235. The Git stuff
  236. =============
  237. General commit rules
  238. --------------------
  239. 1. The commit message should contain either ``[bugfix]`` or ``[feature]``
  240. or ``[refactoring]`` or ``[other]``. In practice however this is very
  241. often forgotten and a commit message like ``fixes #xyz`` is good enough.
  242. Every commit is backported unless
  243. tagged with either ``[feature]`` or with ``[nobackport]``. They are
  244. backported to the latest stable release branch (currently 0.20.x).
  245. Refactorings are backported because they often enable further bugfixes.
  246. 2. If you introduce changes which affect backwards compatibility,
  247. make breaking changes, or have PR which is tagged as ``[feature]``,
  248. the changes should be mentioned in `<changelog.md>`_.
  249. 3. All changes introduced by the commit (diff lines) must be related to the
  250. subject of the commit.
  251. If you change something unrelated to the subject parts of the file, because
  252. your editor reformatted automatically the code or whatever different reason,
  253. this should be excluded from the commit.
  254. *Tip:* Never commit everything as is using ``git commit -a``, but review
  255. carefully your changes with ``git add -p``.
  256. 4. Changes should not introduce any trailing whitespace.
  257. Always check your changes for whitespace errors using ``git diff --check``
  258. or add following ``pre-commit`` hook:
  259. .. code-block:: sh
  260. #!/bin/sh
  261. git diff --check --cached || exit $?
  262. 5. Describe your commit and use your common sense.
  263. Example commit message:
  264. ``Fixes #123; refs #124``
  265. indicates that issue ``#123`` is completely fixed (github may automatically
  266. close it when the PR is committed), wheres issue ``#124`` is referenced
  267. (e.g.: partially fixed) and won't close the issue when committed.
  268. 6. Commits should be always be rebased against devel (so a fast forward
  269. merge can happen)
  270. e.g.: use ``git pull --rebase origin devel``. This is to avoid messing up
  271. git history.
  272. Exceptions should be very rare: when rebase gives too many conflicts, simply
  273. squash all commits using the script shown in
  274. https://github.com/nim-lang/Nim/pull/9356
  275. 7. Do not mix pure formatting changes (e.g. whitespace changes, nimpretty) or
  276. automated changes (e.g. nimfix) with other code changes: these should be in
  277. separate commits (and the merge on github should not squash these into 1).
  278. Continuous Integration (CI)
  279. ---------------------------
  280. 1. Continuous Integration is by default run on every push in a PR; this clogs
  281. the CI pipeline and affects other PR's; if you don't need it (e.g. for WIP or
  282. documentation only changes), add ``[ci skip]`` to your commit message title.
  283. This convention is supported by `Appveyor
  284. <https://www.appveyor.com/docs/how-to/filtering-commits/#skip-directive-in-commit-message>`_
  285. and `Travis <https://docs.travis-ci.com/user/customizing-the-build/#skipping-a-build>`_.
  286. 2. Consider enabling CI (travis and appveyor) in your own Nim fork, and
  287. waiting for CI to be green in that fork (fixing bugs as needed) before
  288. opening your PR in original Nim repo, so as to reduce CI congestion. Same
  289. applies for updates on a PR: you can test commits on a separate private
  290. branch before updating the main PR.
  291. Code reviews
  292. ------------
  293. 1. Whenever possible, use github's new 'Suggested change' in code reviews, which
  294. saves time explaining the change or applying it; see also
  295. https://forum.nim-lang.org/t/4317
  296. 2. When reviewing large diffs that may involve code moving around, github's interface
  297. doesn't help much as it doesn't highlight moves. Instead you can use something
  298. like this, see visual results `here <https://github.com/nim-lang/Nim/pull/10431#issuecomment-456968196>`_:
  299. .. code-block:: sh
  300. git fetch origin pull/10431/head && git checkout FETCH_HEAD
  301. git show --color-moved-ws=allow-indentation-change --color-moved=blocks HEAD^
  302. 3. In addition, you can view github-like diffs locally to identify what was changed
  303. within a code block using `diff-highlight` or `diff-so-fancy`, e.g.:
  304. .. code-block:: sh
  305. # put this in ~/.gitconfig:
  306. [core]
  307. pager = "diff-so-fancy | less -R" # or: use: `diff-highlight`
  308. .. include:: docstyle.rst