add CONTRIBUTING.md

[fakeiteasy]

add CONTRIBUTING.md

[patrik-hagne]

Great, I think we should decide on what to do with tests. The way I've done it is to have one or a couple of MSpec-specifications per feature, these are the ones I start with and have the failing. Once the feature is done the specs will succeed. Then I do normal unit testing while developing.

Is this a workflow you are comfortable with? To me the specs are the ones we can not live without but I like to have unit tests as well.

[adamralph]

Agreed, for a framework such as this the feature tests are the most important. I'm also really keen on unit tests since they always end up improving the implementation ;-) whether written before, after or during the SUT.

I'm quite happy with that workflow. But in the end, CONTRIBUTING.md really just forms rules for how a pull request should look when sent. The specific workflow a contributor uses to get to that point is ultimately up to the contributor. Fine to have this as a suggested workflow though.

[adamralph]

What is the opinion on autocrlf? Opinion in the community is very divided so it's difficult to know "what's best". Historically I've used autocrlf=true for most of my repos, but recently I've switched to autocrlf=false since I think it makes more sense.

1. When building on Mono in Linux in other repos I've had to include bash scripts for executing unit tests and nuget packaging. I always test the Mono build in Windows but also in Linux by sharing my code folder with a Linux VM. If the bash scripts contain CRLF then they fail to execute (this happened to me today with https://github.com/liteguard/liteguard). Having a clone in Linux using autocrlf=false is risky since if I commit any changes there, I may accidentally include CRLF (e.g. changes based in copy pasting code samples) and then this will cause problems when those files are checked out and re-committed in Windows using autocrlf=true. For this reason it is important to use a consistent setting regardless of OS and as demonstrated autocrlf=true is not appropriate for Linux.
2. A VCS has no business tampering with content. It's an inappropriate responsibility and there also exists a small chance that it can corrupt code (although probably negligible).
3. Developers should not ignore line ending changes and their tooling should reflect this, e.g. diff tools should be set to show line ending changes. Ultimately the responsibility for all changes, including line endings, should lay with the developer and not the VCS.

Having said that, it looks like the FIE repo is currently being committed to using autocrlf=true so this would be a change for us. We would all need to switch to autocrlf=false and refresh our checkouts accordingly.

Regardless of which option we choose, it needs to be reflected in CONTRIBUTING.md.

Thoughts?

[adamralph]

Another one which bit me in my last pull request - tabs vs spaces (https://github.com/FakeItEasy/FakeItEasy/pull/40/files#r2952628).

My vote is for spaces.

It looks like the C# files are already using spaces and it's just some other files such as the build script which are currently using tabs. The nuspec has a mix of the two :-p

[patrik-hagne]

It should be spaces, I think I was using Notepad++ for some xml-files and that was set to use tabs I guess.

[patrik-hagne]

I have no real input on this, anyway is fine with me. You decide!

[adamralph]

Ok, I should be able to pick this up soon. @philippdolder do you have any views on the above?

[adamralph]

Some further points

• Max line width
• StyleCop compliance - enforced? if yes, we should first close #44
• Code analysis warnings - should the pull request be free of them? if yes, we'd have to first close #45
• Resharper suppressions in comments - do we allow them?
• Raising issues before starting working on features/bugs
• Indicating intention to work on issues before starting work to avoid overlapping
• Checking that the pull request builds on TeamCity - if we manage to implement #43

We don't necessarily have to discuss all of these.

[philippdolder]

• Max line width:
  • I can comfortably work with lines of 150-160 characters. I wouldn't have a fixed/enforced max line width though
• StyleCop compliance - enforced? if yes, we should first close #44
  • StyleCop: yes, but tweak the default set a bit. The one of appccelerate (https://github.com/appccelerate/appccelerate/blob/master/source/Appccelerate.Source.stylecop) is a good starting point
• Code analysis warnings - should the pull request be free of them? if yes, we'd have to first close #45
  • We should fix all warnings and the build should fail when there are any.
• Resharper suppressions in comments - do we allow them?
  • Problem here is, that it's individual what is a warning, suggestion etc in R#. We should have a settings file for the solution and encourage people to not suppress R# warnings as they usually give a hint to potential flaws. Suppressions should at least be reviewed carefully.
• Raising issues before starting working on features/bugs
  • Raising an issue before starting any work is a must imo. It helps us stay focused and we see who's working on it with the user assignment.
• Indicating intention to work on issues before starting work to avoid overlapping
  • Yes, definitely
• Checking that the pull request builds on TeamCity - if we manage to implement #43
  • Would be great, but is not a high prio thing for me. I'm running a TeamCity server at my company (bbv.ch) that hosts open source projects where bbv employees participate. I almost have all possibilities here with that infrastructure. Ninject and Appccelerate switched from codebetter to that server because it's way faster. What do you think about a switch to teamcity.bbv.ch in the near future?
• Tabs or spaces:
  • Spaces
• auto.crlf
  • I'm not strongly opinionated about that as I'm lacking experience. But I'm using it set to true in all other projects. Though none of them has Mono support. As I understand autocrlf it should fix the issue if it is set to true. The repo on github is in unix-style, my local repo on windows in windows-style, and the repo on a unix build agent for example should be unix-style as well.
• Spec and unit tests
  • I definitely lilke both of them. My approach to work is the same as Patrick's. Exactly, I don't care if a contributor uses that workflow, the result of the pull request is what counts. I would also like to see FluentAssertions in our tests to have easier readable asserts. And I'm not a big fan of // arrange - // act - // assert style comments in tests. But I don't want to fight it if you like it this way. The test should be easy to understand without requiring the blocks marked explicitly

That's all for now. I'm back after a hard week of Carnival in Lucerne :D

[adamralph]

@philippdolder thanks for the input! I think we're agreed on most points. I'll see if can get a pull request together soon with a CONTRIBUTING.md.

• Max line width - I propose we state a guideline of 160 chars (which is in fact my usual preference) but not be strict about it. That would be overkill. A few chars over is OK. 5,000 chars is clearly not ;-)
• Resharper suppressions - my preference is not to allow them in source. I don't use Resharper anymore. However, I'm quite happy for us to include a settings file for those who do. I guess #22 will cover this. I won't be able to close that one since I don't have Resharper ;-).
• Checking that the pull request builds - let's leave this one out until we close #43. If teamcity.bbv.ch is faster than codebetter and we can all have admin access to the project then sure, why not? Codebetter can be painfully slow. BTW - yesterday I started playing around with https://travis-ci.org/. It's a truly wonderful service, so much simpler to setup than TeamCity. At the moment it's restricted to Linux agents but I believe Windows agents are in the pipeline. Could be something to consider for the future.
• Spec and unit tests - I quite like AAA comments since any inappropriate mixing of arrangement, action and assertion in the wrong order is immediately avoided. But like you, I'm not religious about it so perhaps we leave that out of the guidelines for now.

And that just leaves the old favourite, autocrlf. As long as we only work in Windows, true is fine. However, as soon as we start to work in Linux, this breaks. I can't have it set to true in Linux because it will break bash scripts which will be required for a Linux build. I guess we could just run our Mono build in Windows, but my preference would be to run it on Linux as well, to prove the cross platform capability. So I believe we need to answer the following questions:

1. Do we want to officially support Mono?
2. If we do want to officially support Mono, do we need to have a Mono build running under Linux?

If the answer to both these questions is yes, then I think we have no option but to insist that contributors use autocrlf = false. Otherwise, autocrlf = true is OK.

[philippdolder]

To find out if it's worth officially supporting Mono I asked on Twitter (https://twitter.com/philippdolder/status/301967811557855232)

[patrik-hagne]

I like the AAA-comments not for myself but for beginners. I think FIE should be a place that promotes TDD to beginners, that's the sole purpose of it really.

Agree with not allowing Resharper suppressions. I don't use it either and no one should. ;-)

[adamralph]

In that case, let's include the AAA comment guideline.

[philippdolder]

What tool are you guys using to support refactoring?

[patrik-hagne]

I've always managed with the built in tools. I was of course not serious in that no one should use it though, I'm all for choice! The only thing I miss from R# is the navigation features.

[philippdolder]

I couldn't live without R# anymore. All the smart templates, refactoring, test runners etc.

[adamralph]

I'm also happy with the built in stuff, plus some other bits that come from other free and lightweight add-ins, plus my own keyboard-fu. I've always had a love/hate relationship with Resharper. If it was free I might use it but I'm not going to pay for it for personal use. When I first stopped using it I felt a bit lost but I've got used to living without it now and I don't miss it.

I don't like the test runner. I used to, but it's fundamentally flawed. The GUI is nice but it doesn't work properly with xUnit.net Theory tests which generate multiple test commands per method and for the same reason doesn't handle xBehave.net Scenario tests properly either. It also suppresses console output. These days I've fallen back to Testdriven.net inside the IDE and xunit.console outside.

[cecilphillip]

Wouldn't you all be eligible for the open source license since you're the maintainers of an open source project?
www.jetbrains.com/resharper/buy/buy.jsp

[adamralph]

Possibly, but as I said above, I don't really miss it and I always had a love/hate relationship with it anyway. It can be harmful as well as helpful.

[patrik-hagne]

It's definitely so that we're eligible for the OS-license. I've had such a license earlier but I don't use it anymore. If anyone needs it it's easy to apply for it. The same goes for other products such as the profiler.

[adamralph]

@FakeItEasy/owners when time permits, can you please take a look at the first draft and add any comments https://github.com/adamralph/FakeItEasy/blob/36-contributing/CONTRIBUTING.md

[adamralph]

BTW - I had some thoughts about autocrlf and I think it's simpler if we have the guideline as autocrlf=true for now. This is the default setting in Windows and it appears to be what everyone has used to date for FIE since the repo seems to only contain LF and no CRLF.

If we do choose to support Mono officially, I have some ideas which would hopefully remove the need to use bash scripts in Linux/OSX, in which case CRLF on those platforms should cause no issues. This would allow us to state autocrlf=true as the guideline for all contributors, regardless of OS. Another possibility is to state a guideline of autocrlf=input for Linux/OSX clones but hopefullly this won't be required.

NOTE: should we ever wish to do so, I believe it is practical to change the guideline later from true to false, but not from false to true. The latter would cause too much disruption.

[philippdolder]

sounds reasonable to me