Skip to content

[RFC] Extend contributing guide#4585

Closed
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:patch-1
Closed

[RFC] Extend contributing guide#4585
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:patch-1

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Jul 9, 2016

Some might remember that I tried to improve the contribution guide a couple of months ago (#4128). In the end it was decided to move the contribution guide to D wiki, but there are a couple of things that lead me to propose a reconsideration:

  • I have seen that both
    dmd and druntime provide extensive contribution guides focused on their project (they were formerly a bit hidden in HACKING.md)
  • My observation is that many newcomers don't read this guide (e.g. it contains infos about the changelog, AutoTester, coding style, documentation, ...), so we need a better visibility
  • The wiki page hasn't been edited since 2016-03-30 (when I moved it there)
  • Some info in the guide is already outdated and the more visible the guide is, the better the chances of someone updating it. (e.g. @burner tried to update this document, not the wiki)

I believe that the "noise" of having to update the contributing guide from time to time is definitely worth the better experience for newcomers.

That being said, this guide already needs more work, which I am happy to start provided that we agree that having a more extensive CONTRIBUTING.md is a nice thing.

@wilzbach wilzbach changed the title Extend contributing guide [RFC] Extend contributing guide Jul 9, 2016
CONTRIBUTING.md Outdated
* Review Phobos additions in the [Review Queue](http://wiki.dlang.org/Review_Queue).
If you need help you can ask questions on `#d` IRC channel on
freenode.org ([web interface](https://kiwiirc.com/client/irc.freenode.net/d))
or on [our forum](http://forum.dlang.org/).
Copy link
Member

Choose a reason for hiding this comment

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

I would link to the Phobos subsection of the forum.
However, it is currently cluttered by dupes of github notifications. DMD moved away from that a couple of months ago, and druntime and phobos probably should as well. What do you think @braddr ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See also this discussion on the NG about unifying these forums to one.

@Geod24
Copy link
Member

Geod24 commented Jul 9, 2016

Overall I think it's a good idea to move from wiki to CONTRIBUTING.md, for the same reason you outlined for the DIP (collaboration, better visibility...).

I pointed out a couple of things I think should be avoided: some are 'current state' tips, others are still blurred line, and lastly, I would recommend to trim git tips to a minimum.

CONTRIBUTING.md Outdated
- Avoid unnecessarily importing other Phobos modules
- Try to not require a garbage collector (this is not a must)
- Avoid code duplication
- Maximal `@nogc`, `@safe`, `pure`, `nothrow` in *non-templated*
Copy link
Member

Choose a reason for hiding this comment

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

Many of these things belong in D style guide I think

@wilzbach
Copy link
Contributor Author

wilzbach commented Jul 9, 2016

Also, I don't think a single contributor (except Walter and Andrei) should be mentioned in here. Package/module attribution wasn't decided yet (unless I missed something) and I recommend not to make it part of this P.R.

Yes you are right, my last status is that there was consensus that review process needs to be changed. Andrei lists "Better management of language and phobos development, consensus on strategies" and "bot for pinging reviewers (mention-both, highfive)" in the 2016H2 vision.

Overall I think it's a good idea to move from wiki to CONTRIBUTING.md

Thanks for the feedback :)
I started to address the feedback, but I guess we have to go multiple rounds.

CONTRIBUTING.md Outdated
PRs?
- Is your code flexible enough to accommodate more use cases?
- Read through your code again - is there any part that isn't understandable?
- Try to enable `@nogc` tests
Copy link
Member

Choose a reason for hiding this comment

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

Not only @nogc

@wilzbach
Copy link
Contributor Author

Now that dlang/dlang.org#1422 is in, this improvement to the contribution guide can go into another round of review ;-)

@codecov-io
Copy link

codecov-io commented Jul 28, 2016

Current coverage is 88.68% (diff: 100%)

Merging #4585 into master will increase coverage by <.01%

@@             master      #4585   diff @@
==========================================
  Files           121        121          
  Lines         73827      73827          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          65471      65472     +1   
+ Misses         8356       8355     -1   
  Partials          0          0          

Powered by Codecov. Last update 6db08d3...39af376

@wilzbach
Copy link
Contributor Author

So does anyone find more nitpicks or can we use this as an initial version?

@CyberShadow
Copy link
Member

CyberShadow commented Aug 11, 2016

Phew, am I'm glad I saw this.

IMHO:

186 lines? Are you trying to scare everyone away from contributing to D?

What I think: it takes no effort at all to produce mountains and mountains of guides and "recommendations" and "best practices". If I want to submit a small patch to a project (and many contributors start with small patches), and I'm hit with a wall of text that I'm being told I must read before I submit any changes, there's a 99% chance I'm just going to nope out.

My recommendation:

Slim it down to the absolute minimum.

What this should include:

  • Information without which it's not possible for a competent, experienced developer to make a PR, assuming there even are things that fit into that description.
  • Links where curious people can read up on more details, if they want to.

What this should NOT include:

  • DO NOT tell people that they must read the entire style guide. This is grammar-nazi BS, except worse. Now that the style is enforced by CI, just let the bot point it out to them and link to the style guide as necessary.
  • DO NOT tell people that they must run the tests locally. This is what CI is for. Doing so only creates additional hoops that people will need to jump through. The test suite can still be very difficult to run on e.g. Windows.
  • Do not burden people with trivia such as "Trivial changes (e.g. small documentation enhancements) should be marked with [Trivial] as title prefix". The title can be edited by any committer, and competent contributors should pick up the hint. It's fine to have a document that describes our conventions for the sake of consistency and writing it down, but this should not be in the first contributors' guide.
  • Stop burdening people with checklists, generally. We are not bureaucrats.
  • Please don't link to https://wiki.dlang.org/Naming_conventions from here or anywhere. It's a single-purpose research page to help resolve some one-time naming issues.

Yes, I realize that some people must be tired of pointing out common mistakes to newcomers over and over. However, this is not the solution. We will lose more (in potential contributors) this way than from the time saved by creating RTFM-like barriers like this. A better solution is better automation which detects these mistakes and assists the users in fixing them.

@wilzbach
Copy link
Contributor Author

186 lines? Are you trying to scare everyone away from contributing to D?

It wasn't my intention, but your points make sense. If I find time, I will come back to this, but not for now.

Yes, I realize that some people must be tired of pointing out common mistakes to newcomers over and over. However, this is not the solution. We will lose more (in potential contributors) this way than from the time saved by creating RTFM-like barriers like this. A better solution is better automation which detects these mistakes and assists the users in fixing them.

Working on that ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants