[TODO] guidelines for contributing PR's, code style guide, guidelines for issues, forum #8271

timotheecour · 2018-07-11T00:53:59Z

this doc is intended to grow over time to make it easier to keep track of best practices

see also https://forum.nim-lang.org/t/4099 (How should I contribute documentation?): need a section on runnableExamples

Araq · 2018-07-11T06:55:45Z

contributing.md

+
+rationale:
+* proc are more hygienic
+* macros don't work well with UFCS


They work just fine with "UFCS" (which is called "method invocation syntax" in Nim. Why? Because there is nothing uniform in special casing the first argument). What doesn't work fine with UFCS is untyped.

Araq · 2018-07-11T06:56:43Z

contributing.md

+rationale:
+* proc are more hygienic
+* macros don't work well with UFCS
+* templates and macros don't appear in stackrace, making debugging harder


stacktraces

ghost · 2018-07-11T10:03:14Z

contributing.md

+as general rule of thumb, a proc should be preferred over a template, and a template should be preferred over a macro.
+
+rationale:
+* proc are more hygienic


Maybe "procedures" instead of "proc" here?

dom96 · 2018-07-11T20:57:46Z

This document should link to NEP1 instead of repeating it partially (I'm referring to the proc vs. template vs. macro thing here).

Also, please see this: https://help.github.com/articles/setting-guidelines-for-repository-contributors/. There are some really good examples there that we should simply copy. I prefer the Open Government one as it's short. The idea for this document is to let people know about contributing, not about the code style or what the Nim forum is good for (we have documents for that already). This document needs to be short and to the point.

timotheecour · 2018-07-11T23:34:11Z

@dom96 thanks for mentioning NEP1 (https://nim-lang.org/docs/nep1.html), I'll fix that PR to avoid repeating what's in there (also referring to it in contributing.md would increase visibility of NEP1)

This document needs to be short and to the point

Not sure where's the best place to put this but what I wanted was a central easy to find document/folder that contains (or points to other docs) with best practices (prefer X over Y, with a rationale for that, sometimes pros/cons)
It'd be analog to https://google.github.io/styleguide/cppguide.html but mostly concerned with semantic choices (not too many syntax/whitespace issues in Nim thankfully, although https://nim-lang.org/docs/nimfix.html would make that syntax discussion even smaller); it would not be about explaining the language semantics (for which we already have a guide).

That document should be comprehensive as opposed to minimal to help users in common situations (both for Nim repo but should be relevant for 3rd party code); example:

when to use assert vs doAssert (eg I see both used in isMainModule unittest blocks)
runnableExamples vs tests in isMainModule vs tests outside of module

It should be continuously updated as a single authoritative source of best practice ; eg having a choose X over Y in there is often more practical than deprecating Y (to avoid breaking code that depends on Y)

Maybe contributing.md is not the right place for that?
How about something like doc/best_practices.rst (next to doc/nep1.rst for NEP1), and linking to that in contributing.md?

GULPF · 2018-07-12T05:29:06Z

contributing.md

+* try to use `runnableExamples:` block to make generated docs more illustrative and for testing purposes
+
+### unit tests
+* more extensive testing (eg that would be overkill for docs) can be done in `when isMainModule` blocks right below the proc:


This is not what we do now (currently all additional tests go into a single isMainModule block at the bottom of the file), and IMO it shouldn't change. Typically when you're working on a module you're interested in either the implementation or the tests, not both at the same time. Having to read code that is a 50/50 split of tests and implementation is just painful.

Also, the main thing that should be mentioned in this section is testament and how to use it

andreaferretti · 2018-07-12T07:14:38Z

contributing.md

+
+## git
+### use `git rebase` instead of `git merge`
+In command line (for person doing PR):


I don't care one way or the other, but is this currently one of the guidelines used when contributing? I feel this PR should document the guidelines and best practices that are in place, not invent new ones

To be more clear: I think that imposing all this git trickery will only discourage useful PRs. I think I have seen many PRs that were not rebased or squashed, but were accepted anyway

andreaferretti · 2018-07-12T07:18:30Z

contributing.md

+### prefer `any` or a explicit or generic type over `untyped` for `template` and `macro` parameters when possible
+rationale: early type checking makes it easier to debug errors
+
+###  use `void` instead of `typed` for `template` and `macro` output


I don't think this suggestion is very clear. void means - well, no return type. As such, it can only be used when the template does not return anything, or so I think. If I have, say, template foo(): untyped = 5, I should change that untyped into int, not void

Araq · 2018-07-12T10:27:33Z

We already have: https://github.com/nim-lang/Nim/blob/devel/doc/contributing.rst

Please add your ideas to this document instead. And yes, we use .rst because that's what Nim's default is.

dom96 · 2018-07-12T10:51:21Z

Maybe contributing.md is not the right place for that?
How about something like doc/best_practices.rst (next to doc/nep1.rst for NEP1), and linking to that in contributing.md?

I don't think it is the right place. In fact, NEP1 seems like a fine place for this too. It explains all code style, this could include doAssert vs. assert (although for this particular it might be better to explain it in the assert/doAssert doc comment, if it's not already explained there).

dom96 · 2018-07-12T10:52:07Z

Please add your ideas to this document instead. And yes, we use .rst because that's what Nim's default is.

This is all well and good, but GitHub shows a link to a contributing.md file when people make a PR. This might also work for contributing.rst so maybe we just need to move it to the repo root.

Araq · 2018-07-12T11:24:58Z

This is all well and good, but GitHub shows a link to a contributing.md file when people make a PR. This might also work for contributing.rst so maybe we just need to move it to the repo root.

That bites with your RFC of removing top level files though. And indeed, it feels messy, so I agree with your RFC.

haltcase · 2018-07-12T13:17:07Z

You can store these kinds of files in a .github or docs folder in the repo root. See here, first paragraph.

dom96 · 2018-07-12T18:51:22Z

So I guess we have to rename our doc folder to a docs folder :)

…es, macros

…(adapted from nim-lang#8315 (comment))

narimiran · 2018-07-23T07:32:15Z

contributing.md

+proc myStringify(a: int) : string =
+  ## Stringifies ``a``.
+  ##
+  ## On Windows, follows this [spec](https://en.wikipedia.org/wiki/Hidden_file_and_hidden_directory).


Isn't this mixing of .rst syntax (double backticks) and .md syntax (link)? Will this work as intended?

narimiran · 2018-07-23T07:32:51Z

contributing.md

+    doAssert foo(0) == "0"
+  result = $(a)
+```
+* we use double backticks (bolds text in RST) as ooposed to single backticks (italicizes it)


typo (ooposed)

Araq · 2018-08-13T23:08:29Z

Well we have a contributing guide already, apparently you haven't read it (I'm not blaming you at all!). It's fair to assume this one won't be read either. Contributing should be simple/easy/fast, long documents how to do it are more counter productive than helpful.

dom96 · 2018-08-14T09:19:09Z

Yep, this is far too long. As I've mentioned previously, if we really want this then let's just adopt one of these (Open Government is my favourite): https://help.github.com/articles/setting-guidelines-for-repository-contributors/

timotheecour force-pushed the pr_contributing branch from 0256514 to 28cc3f6 Compare July 11, 2018 00:54

timotheecour closed this Jul 11, 2018

timotheecour reopened this Jul 11, 2018

Araq reviewed Jul 11, 2018

View reviewed changes

ghost reviewed Jul 11, 2018

View reviewed changes

GULPF reviewed Jul 12, 2018

View reviewed changes

andreaferretti reviewed Jul 12, 2018

View reviewed changes

timotheecour mentioned this pull request Jul 14, 2018

fix #8225 os.isHidden was buggy on posix #8315

Merged

timotheecour added 3 commits July 14, 2018 14:01

added contributing.md

08764a5

address comments, add points regarding input/output params in templat…

b18ed0e

…es, macros

add a sample function that can serve as reference for code to follow …

b370902

…(adapted from nim-lang#8315 (comment))

timotheecour force-pushed the pr_contributing branch from c951290 to b370902 Compare July 14, 2018 21:02

narimiran reviewed Jul 23, 2018

View reviewed changes

Araq closed this Aug 13, 2018

timotheecour changed the title ~~guidelines for contributing PR's, code style guide, guidelines for issues, forum~~ [TODO] guidelines for contributing PR's, code style guide, guidelines for issues, forum Aug 14, 2018

This was referenced Oct 9, 2018

better docs for tables module #9221

Merged

make contributing.rst more up to date; reference it in readme.md to make it more discoverable #9302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TODO] guidelines for contributing PR's, code style guide, guidelines for issues, forum #8271

[TODO] guidelines for contributing PR's, code style guide, guidelines for issues, forum #8271

timotheecour commented Jul 11, 2018 •

edited

Loading

Araq Jul 11, 2018 •

edited

Loading

timotheecour Jul 11, 2018

Araq Jul 11, 2018 •

edited

Loading

timotheecour Jul 11, 2018

ghost Jul 11, 2018

timotheecour Jul 11, 2018

dom96 commented Jul 11, 2018 •

edited

Loading

timotheecour commented Jul 11, 2018 •

edited

Loading

GULPF Jul 12, 2018 •

edited

Loading

andreaferretti Jul 12, 2018

andreaferretti Jul 12, 2018

andreaferretti Jul 12, 2018

andreaferretti Jul 12, 2018

Araq commented Jul 12, 2018

dom96 commented Jul 12, 2018

dom96 commented Jul 12, 2018

Araq commented Jul 12, 2018

haltcase commented Jul 12, 2018

dom96 commented Jul 12, 2018

narimiran Jul 23, 2018

narimiran Jul 23, 2018

Araq commented Aug 13, 2018

dom96 commented Aug 14, 2018

[TODO] guidelines for contributing PR's, code style guide, guidelines for issues, forum #8271

[TODO] guidelines for contributing PR's, code style guide, guidelines for issues, forum #8271

Conversation

timotheecour commented Jul 11, 2018 • edited Loading

Araq Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Araq Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dom96 commented Jul 11, 2018 • edited Loading

timotheecour commented Jul 11, 2018 • edited Loading

GULPF Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Araq commented Jul 12, 2018

dom96 commented Jul 12, 2018

dom96 commented Jul 12, 2018

Araq commented Jul 12, 2018

haltcase commented Jul 12, 2018

dom96 commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Araq commented Aug 13, 2018

dom96 commented Aug 14, 2018

timotheecour commented Jul 11, 2018 •

edited

Loading

Araq Jul 11, 2018 •

edited

Loading

Araq Jul 11, 2018 •

edited

Loading

dom96 commented Jul 11, 2018 •

edited

Loading

timotheecour commented Jul 11, 2018 •

edited

Loading

GULPF Jul 12, 2018 •

edited

Loading