Improve contribution guide #4128

wilzbach · 2016-03-29T12:23:36Z

In reference to the forum thread, I propose some hints and advice for new contributors. The current list resembles stuff that I learned over the last weeks, feel fee to add your own insights ;-)
This guide should also inform new users about undocumented (or hard to find) policies like pinging people or std.experimental.

I used - for all bullets as the Github markdown checklist syntax only supports - [ ] and I wanted to have only one type

Preview.

PetarKirov · 2016-03-29T13:33:05Z

CONTRIBUTING.md

+### Code
+
+- Don't add `@nogc`, `@pure`, `@safe`, `nothrow` attributes yourself - let the compiler infer it! However add unittest check for these attributes if possible


All those attributes should be put on non-templated functions manually. You should make it clear that the above statement only applies to templates.

PetarKirov · 2016-03-29T13:51:29Z

You should also make a note that all user impacting changes should have a corresponding changelog entry.
Bug fix PRs should have a separate commit for each bugzilla issue that it fixes. The message of the commit actually fixes the bug should start with "Fix issue XXXXX" where XXXXX is the number of the corresponding bugzilla issue. For examle:

+ Refactor the code assist with the bug fix
+ Fix issue 17000 - map doesn't accept  some range types
+ Fix issue 17001 - map doesn't work with some other range type

Tightly related fix are OK to go in the same PR. Unrelated fixes/refactorings/additions should preferably go separate pull request(s).

quickfur · 2016-03-29T15:51:58Z

CONTRIBUTING.md

+- Fork [on Github](https://github.com/D-Programming-Language/phobos)
+- Use our [Bugzilla bug tracker](http://d.puremagic.com/issues/)
+- Follow the [Styleguide](http://dlang.org/dstyle.html)


Nitpick: "style guide" should be two words.

quickfur · 2016-03-29T16:05:22Z

CONTRIBUTING.md

+- [Autotester](http://wiki.dlang.org/Git_Commit_Tester) will automatically compile the code in your PR and test it on all supported platforms. For you first PR you need approval from any reviewer - ping them in case they forget.
+- [ ] Do all tests pass? You can run the tests of a single module with `rdmd -main -unittest module.d`, of an entire package with `make -f posix.mak std/algorithm.test` and all tests with `make -f posix.mak unittest`
+- [ ] Do your tests cover all cases? (you can check code coverage in the resulting `module.lst` of `rdmd -cov -main -unittest module.d`)


Might want to mention that it's useful to run tests locally as well, to prevent waste of autotester cycles by silly mistakes like typos and syntax errors, or code that obviously doesn't work even locally. Phobos unittests can be run by:

make -f posix.mak unittest

But that takes a long time, so I often just run:

dmd -main -unittest std/algorithm/iteration.d -of/tmp/test && ./test; echo $?

and it will print 0 if all goes well, otherwise there will be an error message or a non-zero error code (usually means a crash).

Of course, this is on Posix only... Windows users probably would have a different workflow.

wilzbach · 2016-03-29T16:12:43Z

@ZombineDev incorporated all your feedback & I added a couple of other items to the list.

@quickfur

Nitpick: "style guide" should be two words.

Changed to "the D style".

I thought auto return types should be encouraged, as per the previous note on Voldemort types?

AFAIK the idea of explicitly specifying the return type if possible is that it is easier to read (documentation and code)

Nested structs generally should be marked static unless there's a good reason they need to close over the enclosing function's local variables. (As a rule, function parameters should be passed to the ctor and stored explicitly in the struct rather than using a closure -- this is to prevent breakage in certain cases where a struct that closes over local variables may not work properly downstream with other range algorithms).

I tried to shorten this a bit. Hope you still like it, otherwise criticize again ;-)

Furthermore, if the struct needs to access alias template arguments, it probably should be put outside the function as a private module-global struct instead. This is to work around certain compiler bugs/limitations that may cause compilation errors otherwise. I'm not sure if this has since been fixed, but it's something to keep in mind.

Does anyone have more information on this?

Update:
@quickfur

Might want to mention that it's useful to run tests locally as well, to prevent waste of autotester cycles by silly mistakes like typos and syntax errors, or code that obviously doesn't work even locally.

These instructions are already there - I recommend to use rdmd -main -unittest though.

quickfur · 2016-03-29T16:53:44Z

Might also want to mention what to do if the auto-tester comes back with a merge failure?

quickfur · 2016-03-29T16:55:08Z

CONTRIBUTING.md

+- Use `$(REF_ALTTEXT)` for a different link name (format is `$(REF_ALTTEXT your text, formattedRead, std, format)`
+- Variables will be automatically put in backticks, use an underscore to avoid this behavior (e.g. `_foo`)
+- Don't put examples in your ddoc header - use the `///` magic to annotate them.


It's not clear (to a newbie) what "/// magic" means. Might want to give an example + short explanation?

wilzbach · 2016-03-29T18:08:58Z

All comments addressed.

Might also want to mention what to do if the auto-tester comes back with a merge failure?

I added the section on rebasing and squashing from Starting as a Contributor and modified it a bit - so all is in one document.

quickfur · 2016-03-29T18:12:05Z

LGTM, thanks for taking the time to improve this very important document! I'll wait a bit for other reviewers to take a look, then it's merge time!

wilzbach · 2016-03-29T18:15:07Z

LGTM, thanks for taking the time to improve this very important document!

The pleasure is on my side ;-)

I'll wait a bit for other reviewers to take a look, then it's merge time!

I hope that other reviewers can recall some problems they had in their newbie time or common errors they criticize on PRs.

PetarKirov · 2016-03-29T19:16:22Z

CONTRIBUTING.md

+unittest
+{
+	assert(1 == 1);


Niptick: use four spaces, instead of one tab.

JackStouffer · 2016-03-29T19:32:49Z

I applaud your work here, but I argue that this is the wrong place for it.

About four months ago @CyberShadow and I consolidated all contriduting information (it was strewn about five or so different places) into one article: http://wiki.dlang.org/Starting_as_a_Contributor

I would like it if that article stayed the definitive source on contributing, as spreading everything around lead to info being forgotten about and becoming outdated. So, please add this information there instead.

PetarKirov · 2016-03-29T19:42:37Z

CONTRIBUTING.md

+### Tests
+
+- [Autotester](http://wiki.dlang.org/Git_Commit_Tester) will automatically compile the code in your PR and test it on all supported platforms. For your first PR you need approval from any reviewer - ping them in case they forget.


"all supported platforms" - technically not all, only most of the supported (x86 and x86_64 platforms). Maybe we should add a note to consult with the other compiler devs (GDC and LLVM) about low-level changes that may make cross-compiler portability more difficult.

Rephrased to "All PRs must pass all tests for all supported platforms of [autotester]" and added the line about GDC/LLVM devs.

PetarKirov · 2016-03-29T20:11:25Z

CONTRIBUTING.md

+1. [Fork](https://help.github.com/articles/fork-a-repo/) [Phobos](https://github.com/D-Programming-Language/phobos) on Github
+2. Create your [own branch](https://github.com/Kunena/Kunena-Forum/wiki/Create-a-new-branch-with-git-and-manage-branches)
+3. Work locally on your new feature or fix (see the tips below)


Maybe a good place to link to building and locally testing?

+1 done. However I kept the link to tests and link there to locally testing. The new tests section provides some helpful advice like using rdmd -main -unittest for single modules.

wilzbach · 2016-03-29T20:15:11Z

I applaud your work here, but I argue that this is the wrong place for it.

I disagree with you for the following reasons

Every time someone opens a pull request, he sees a link to this document
This document is part of the Phobos repository, giving it an official status, so guidelines have an official, trusted touch (e.g ping @andralex` or format your PRs like this)
The starting as a contributor guide covers all aspects of DLang - this one here is specially targeted at Phobos. I myself admit that I just quickly scrolled over your carefully written guide in the beginning because much didn't seem relevant to me
If you have a clone of Phobos, you automatically have a copy of this document

About four months ago @CyberShadow and I consolidated all contriduting information (it was strewn about five or so different places) into one article: http://wiki.dlang.org/Starting_as_a_Contributor

Kudos!

I would like it if that article stayed the definitive source on contributing, as spreading everything around lead to info being forgotten about and becoming outdated. So, please add this information there instead.

For the argumentation above I would prefer the other way around: removing the Phobos part and add it here.

PetarKirov · 2016-03-29T20:17:09Z

@JackStouffer I think that most of what @greenify has written in this PR doesn't overlap with the information on the wiki. The other repositories would also benefit from specific contribution guides. I think that common information should be consolidated (maybe on the wiki) and the specific guides should be in each repository.

PetarKirov · 2016-03-29T20:21:31Z

@greenify I saw that you addressed all of my comments. Nice work!
LGTM

JackStouffer · 2016-03-29T21:04:03Z

@greenify Please see the discussion here: #3832

If the documentation for this sort of thing isn't all in one place, it will become outdated and people will forget to update it. This is a slippery slope, if we start putting the info in different places again it will snowball.

Every time someone opens a pull request, he sees a link to this document

And the first thing in this document is a link to the wiki article in question

The starting as a contributor guide covers all aspects of DLang - this one here is specially targeted at Phobos. I myself admit that I just quickly scrolled over your carefully written guide in the beginning because much didn't seem relevant to me

The vast majority of the document is necessary for even opening a simple PR and testing it.

JackStouffer · 2016-03-29T21:05:17Z

@ZombineDev

I think that most of what @greenify has written in this PR doesn't overlap with the information on the wiki.

All the more reason for it to be added to the wiki page.

quickfur · 2016-03-29T23:22:49Z

I agree that any info missing from the wiki page ought to be added there.

However, I think a case can be made for a "quick start to contributing to Phobos" type of document in place of our current CONTRIBUTING.md file. Perhaps the solution here is to simplify this PR to a short list of small bullet points that can be glanced over quickly, with a prominent link (or three) to the wiki page for further details. Anything that's currently in this PR that isn't already in the wiki page should be added to the wiki page.

rcorre · 2016-03-30T00:28:51Z

CONTRIBUTING.md

+- Maximal `@nogc`, `@safe`, `pure`, `nothrow` in _non-templated_ functions
+- Don't add `@nogc`, `@safe`, `pure`, `nothrow` attributes to _templated_ functions - let the compiler infer it! However add unittest checks for these attributes if possible (e.g. `pure nothrow @nogc @safe unittest { ... }`)
+- Avoid to use `auto` as return type wherever possible (makes the documentation easier to read)


Nitpick: 'avoid using' instead of 'avoid to use'.

CyberShadow · 2016-03-30T05:13:19Z

I'm sorry, but @JackStouffer is right.

We have code not in one repository, but several. Lacking a way to transclude Markdown files across repositories, our only options are links or copy-paste. Before everything was consolidated into one article, everything was a huge mess with conflicting or outdated information all over the place. As such, merging this pull request would be a regression.

The wiki is not any less official than the markdown file, and such text guides are better hosted on the wiki anyway, since they do not warrant the high contributing bar that a pull request entails.

Please integrate any relevant information into the wiki and close this pull request.

wilzbach · 2016-03-30T10:46:44Z

Please integrate any relevant information into the wiki and close this pull request.

https://wiki.dlang.org/Contributing_to_Phobos

CyberShadow · 2016-03-30T10:53:19Z

https://wiki.dlang.org/Contributing_to_Phobos

Nice, maybe link to that directly from CONTRIBUTING.md?

wilzbach · 2016-03-30T10:54:22Z

Nice, maybe link to that directly from CONTRIBUTING.md?

Already preparing the PR ;-)

PetarKirov · 2016-03-30T15:44:16Z

@CyberShadow the process of using the wiki for important documents seems to me like a regression. Wiki pages don't get reviewed before published, which is a major disadvantage, IMO. Just look at all thr improvements that @greenify as a result of this PR review.
It would be much better if we had a single git repository (part of the D-Programming-Language organization) for such things. For example, rust has benefited to a great extent from this process: https://github.com/rust-lang/rfcs/blob/master/README.md.

CyberShadow · 2016-03-30T16:02:34Z

@CyberShadow the process of using the wiki for important documents seems to me like a regression.

The actually important documents, such as the language spec, are hosted on dlang.org.

Wiki pages don't get reviewed before published, which is a major disadvantage, IMO.

We do not have the resources to review typo / dead link fixes.

Look at the page's history: https://wiki.dlang.org/?title=Get_involved&action=history

Could you really see every edit (series of successive edits by one editor) as a separate pull request?

Just look at all thr improvements that @greenify as a result of this PR review.

Yes. Or you could watch the wiki page. Email notifications are enabled.

For example, rust has benefited to a great extent from this process: https://github.com/rust-lang/rfcs/blob/master/README.md.

Where's the Rust RFC that teaches people on how to use git and github?

PetarKirov reviewed Mar 29, 2016
View reviewed changes

quickfur reviewed Mar 29, 2016
View reviewed changes

wilzbach force-pushed the improve_contribution_guide branch 2 times, most recently from 5a28566 to 04d74be Compare March 29, 2016 15:59

quickfur reviewed Mar 29, 2016
View reviewed changes

wilzbach force-pushed the improve_contribution_guide branch from 04d74be to 71dd91c Compare March 29, 2016 16:12

wilzbach force-pushed the improve_contribution_guide branch 2 times, most recently from 1818a69 to 608cd81 Compare March 29, 2016 16:25

quickfur reviewed Mar 29, 2016
View reviewed changes

wilzbach force-pushed the improve_contribution_guide branch 3 times, most recently from 113dea9 to 50c11d3 Compare March 29, 2016 17:46

wilzbach force-pushed the improve_contribution_guide branch from 50c11d3 to 8a599db Compare March 29, 2016 18:09

wilzbach force-pushed the improve_contribution_guide branch from 8a599db to c91d90d Compare March 29, 2016 18:12

PetarKirov reviewed Mar 29, 2016
View reviewed changes

CONTRIBUTING.md

unittest

{

assert(1 == 1);

Copy link

Member

PetarKirov Mar 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Niptick: use four spaces, instead of one tab.

PetarKirov reviewed Mar 29, 2016
View reviewed changes

wilzbach force-pushed the improve_contribution_guide branch from c91d90d to 5f34746 Compare March 29, 2016 20:04

PetarKirov reviewed Mar 29, 2016
View reviewed changes

wilzbach force-pushed the improve_contribution_guide branch from 5f34746 to b0cc731 Compare March 29, 2016 20:19

rcorre reviewed Mar 30, 2016
View reviewed changes

wilzbach force-pushed the improve_contribution_guide branch from b0cc731 to ea9ac68 Compare March 30, 2016 04:39

Improve contribution guide

ce8ec08

wilzbach force-pushed the improve_contribution_guide branch from ea9ac68 to ce8ec08 Compare March 30, 2016 07:10

wilzbach closed this Mar 30, 2016

wilzbach mentioned this pull request Mar 30, 2016

Link from CONTRIBUTING.md to the new wiki contribution guide #4133

Merged

wilzbach mentioned this pull request Jun 18, 2016

Link to Walter, Andrei Action List #4435

Closed

wilzbach mentioned this pull request Jul 9, 2016

[RFC] Extend contributing guide #4585

Closed

		### Code

		- Don't add `@nogc`, `@pure`, `@safe`, `nothrow` attributes yourself - let the compiler infer it! However add unittest check for these attributes if possible

		### Tests

		- [Autotester](http://wiki.dlang.org/Git_Commit_Tester) will automatically compile the code in your PR and test it on all supported platforms. For your first PR you need approval from any reviewer - ping them in case they forget.

Uh oh!

Improve contribution guide #4128

Improve contribution guide #4128

Uh oh!

Conversation

wilzbach commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 29, 2016

Uh oh!

quickfur commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 29, 2016

Uh oh!

quickfur commented Mar 29, 2016

Uh oh!

wilzbach commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 29, 2016

Uh oh!

PetarKirov commented Mar 29, 2016

Uh oh!

PetarKirov commented Mar 29, 2016

Uh oh!

JackStouffer commented Mar 29, 2016

Uh oh!

JackStouffer commented Mar 29, 2016

Uh oh!

quickfur commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Mar 30, 2016

Uh oh!

wilzbach commented Mar 30, 2016

Uh oh!

CyberShadow commented Mar 30, 2016

Uh oh!

wilzbach commented Mar 30, 2016

Uh oh!

PetarKirov commented Mar 30, 2016

Uh oh!

CyberShadow commented Mar 30, 2016

Uh oh!

Uh oh!