What should go into contributor guidelines

[veikkoeeva]

Last time edited on 2015-03-09T19:24:43+00

There isn't a repository contributor guideline on Orleans as of yet. I think it should provided. What do you think should be in it?

Some food for thoughts in the following.The main idea is to make the process scalable, "self-driven" when it comes to daily routines.

Write guidelines for the development and design flow

• Design on branching and tagging model, as discussed at Ideas for contributions #29 (here and here), also touched upon Downgrade dependency on Microsoft.WindowsAzure.ServiceRuntime to 2.4 #197 (surely we shouldn't encourage private forks).
• How to take in external libraries as they increase library conflicts, touched upon at Management of internal and application dependencies #200 and at PowerShell Snap-In Prototype #150 (here).
• A guideline with Git commands and other appropriate helpers from cloning to a pull request and what ought to be checked there. Perhaps there should reasoning for the guidelines. For instance, why history should look certain so as to facilitate making conclusions out of it.
• Experimenting, ideas and so forth aren't frowned upon. Messing up one's branch, core or someuch in a public PR isn't an issue as long as the code ending up the main repo is acceptable. To err is human and everything can be fixed.
• Encourage research extensions?

Qualities and guidelines in order of important (e.g. bug fixes)

1. Security
2. Scalability (& performance)
3. Reliability
4. Maintainability (the Orleans development, debugging and testing story ought to be: clone the repo, hit F5).
5. ???

How to enforce the above mentioned? For instance, security should be important, in my opinion, but I don't know how to systematically and explicitly check security issues other than that if there are two otherwise equally important bugs, I'd fix the one with security problem first. Likewise when developing a new feature, it should developed so as to not expose a vulnerability even if other quality might warrant making such a decision, for instance scalability. That is, perhaps scalability is important and scalability features are designed and implemented, but if security problem is found, it should be fixed.

<edit 2015-03-09T19:13:43+00>
A first draft of Orlean mission statement? By @gabikliot.
</edit>

Reviewing code

• Conforms to .NET Framework Design Guidelines unless a reason argued in comments. When using Visual Studio, Code Analysis could be set to a certain level.
• Prefer checking parameters always on public (internal?) and protected interfaces (perhaps on private too) as per Parameter Design Guidelines and currently point 2. on qualities list. Other way to phrase this is in the language of invariants or pre- and post-conditions, pointing also to Code Contrats. Should they be considered?
• Has comments at least on the public surface. Or also anything that is non-trivial (example).
• Favour making structs, classes and functions immutable (immutable collection parameters, immutable return values or copies) when feasible. Should it commented in the code if, say, a collection given as a parameter will be mutated or a new one given as return value?
• When programming classes, when feasible, favour style where functions take parameters and return values over style where functions (procedures in old Pascal parlance) return void and take no parameters and instead mutate object state. This makes following the code easier, easier to test and consequently more robust and amenable to refactoring.
• How stringent on tests? Should pass, naturally. Core functionality should include, otherwise actively encouraged?

<edit 2015-03-09T19:24:43+00>
Some preliminary guidelines on checking parameters. By @gabikliot.
</edit>

That's about my first round of shedding ideas. Any others? I reckon some of these are controversial, such as the one with functions. What guidelines other .NET repos have?

[jthelin]

Should also explicitly add the requirement for xml code-docs for all public classes / methods.

You may have included this on your point 3 above but in my mind comments [in code, to explain significant implementation choices and design decisions] and code docs are somewhat orthogonal and useful for different reasons.

👍 to doing this project specific guidelines page.

[sergeybykov]

This is a great start. Thank you, @veikkoeeva!

[veikkoeeva]

@jthelin I'll update the first post and timestamp it. What should we consider as public classes and methods? Those marked with the reserved word public or as in broader sense classes and functions that can be used "publicly", which I think would public, protected and internal, but may be too much. One aspect here is that what if clarification is requested, how should we decide is something warrants commenting? Personally I err on the side of more commenting, but others may not consider it worth it. Kind of rules of the game, "good governance", where it'd be clear what everyone can expect. Naturally it shouldn't be too suffocating considering the goals.

@sergeybykov Sure, just don't be afraid to assert your authorship. As you can see, I wrote a list of qualities in a priority order and other things, putting a bit words on your mouth. I'll try to just nudge the project so that on one hand everyone would have a good basis to discuss on more fundamental issues -- like you and other more permanent people -- and on other hand you guys wouldn't need to scrutinize every refactoring and still be assured the code works.

In the same vein, should we consider something like Gitter. I lurk Aurelia on Gitter and I find it rather splendid.

[jthelin]

What should we consider as public classes and methods? Those marked with the reserved word public or as in broader sense classes and functions that can be used "publicly", which I think would public, protected and internal, but may be too much.

@veikkoeeva - Certainly things that use 'public' and 'protected' keywords should have API docs.

For things that use 'internal' keyword, i view API docs as desirable for the "internal component interfaces" -- eg the inter-component surface area that Gabi mentioned in other comment thread.
#202 (comment)

To try to distill it down to "MoSCoW" categories, seems to net out as:

• MUST have for 'public' and 'protected' API members
• SHOULD have for 'internal' members if part of inter-component API
• COULD have for 'internal' members if part of intra-component calls
• WON"T have for 'private' API members

Orthogonal to that, explanatory comments at strategic points in code should always be encouraged if the logic is complex, or where specific design choices made and worth documenting so that others do not repeat effort [eg "Taking snapshot copy of List contents here for safety while we process them", or "Explored using HashSet here but List is better because access is mostly-write and additions are always at-end and read access will be sequential from beginning".

Working out what constitutes "too much" comments is probably something that may need to evolve over time in this group, although in reality that is a professional meta-problem not restricted to Orleans ;)

[jthelin]

The other item that is important to spell out explicitly in Contributor Guidelines is around white-space policy.

For the record:

• We use leading spaces [not tabs]
• Indents set to 4.

While this may be nested inside some of the other guidelines above, but explicit statement can only help to avoid any potential ambiguity in this area.

We do have a .gitattributes file set up in the repo which should handle normalization of end-of-line characters correctly - but for the record it is "supposed" to give platform-native on local machine [ie CRLF on Windows] & normalized to LF inside repo.

* text=auto

There is not supposed to be any "unprintable" characters in any of the source code / text files.
However, some problems in this area did creep in to the repo initially [before PR #31]
Please let us know if you find any more occurrences because the working hypothesis is that these problems instances were fixed in PR #24.

Also, please don't propose any changes that contain "Wall of Pink" -- we like to see the real code changes not get lost in the white-space deltas :)
http://www.hanselman.com/blog/YoureJustAnotherCarriageReturnLineFeedInTheWall.aspx

[veikkoeeva]

@jthelin I'll put some food for thoughts here at the beginning and some observation after that.

I suppose the Orleans contributor guidelines ought to bear resemblance to other Microsoft related guidelines. The CoreFX one is linked on the main page already. Maybe there should be more community related content. For instance, the Azure SDK one lays down some points on community building rather well (as @jthelin's point on pink wall), even if a bit scattered through links:

Contributing code

From an engineering standpoint, please make sure that you can build and test the code. Familiarize yourself with our project guidelines and coding conventions.

The SDK team also recommends that you read these great posts about open source:

• Open Source Contribution Etiquette by Miguel de Icaza
• Don't "Push" Your Pull Requests by Ilya Grigorik
• A Successful Git Branching Model by Vincent Driessen

If you have ever considered contributing to the ASP.NET Web Stack, we have modeled much of this process on that project.

Before you start working on a feature or substantial code contribution please discuss it with the team and ensure it is an appropriate addition to the core product.

Other successful guidelines worth to take a peek at could be [the F# guidelines (see also Becoming a Contributor)](https://github.com/Microsoft/visualfsharp/blob/fsharp4/CONTRIBUTING.md) (albeit its emphasis is different) or the ASP.NET engineering guidelines enumerate rather thorughly the various guidelines with regard to the concrete code, as CoreFX one. There is already concrete Orleans code and perhaps on thing to consider is, which kind of refactoring, if any, is all right, say, to meet .NET Design Guidelines.

The goals of Orleans project and community expectations naturally evolve, especially now in the beginning. I personally would want to make sure you, the project core contributors and maintainers, have enough bandwidth to do the core work and some experimenting, and then make sure improvements get pumped out fairly rapidly. Though the nature of Orleans stack is such it may find usage in areas that stipulate (official) stability, e.g. only minor updates (which may be easier with fairly rapid release cycles).

Related to core work, I would like as many people as possible to do experimental extensions, say, regarding distributed data structures or distributed architectures. To that end, documentation on extension points, making the project "easily approachable" on all levels etc. is important. Perhaps even so as to encourage people to open issues and submit PRs with the explanation that opening PR doesn't necessarily mean it be included in the project, but it's a point of discussion about the code, with perhaps a pointer to Codeplex forums or the Jabbr room. As an added note, I occasionally open PRs with an intent to share some findings on possible avenues to explore or pain points to address, as was the case with the Storage update and conversion of examples. I don't mind if they are discarded or incorporated elsewhere (I'm not able to put that many hours to Orleans, so things linger anyway). If that's all right, maybe it should be mentioned explicitly as a possibility and in my case, to write the intention clearly in the PR.

So, setting the tone and guidelines should be important. I like how @jthelin mentioned MoSCoW on core review is good, it sets the expectation in this particular regard. Regarding PRs, maybe one guideline is that the core maintainers signal approval or somesuch, whereas other involved people could point out issues, but otherwise if none is found, silence means tacit approval.

I personally see it important to mention the mission and intent of the project, and what it means. For instance, in the case of the link, it could mean some churn on the public API with the premise to make it easier to consume. On the other hand, the .NET Framework Design Guidelines facilitate APIs that feel natural to .NET people. Also, as Orleans is at least outside of Microsoft, quite a new framework, some churn on public APIs would be more affordable as of now. Then, perhaps, some kind of a mention on how NFRs are handled when there's a conflict of priorities.

Also, with regard to other platforms, perhaps it would need to be spelled out how to work on the various platforms so as not to at least block them straight out. For instance, maybe it could be spelled that with regards to tooling and testing, it's all right to include code in some special ways to the project that is only usable on some specific platform. For example, so that there are multiple .sln files that include the platform specific .csproj files.

Plus all the issues that contribute to daily work when the hands are on the keyboard, some of which @jthelin mentioned.

Uh, this gets rather long already. But I suppose I'd like some more about this in addition to what is already at the Community Wiki and less of a link to CoreFX and then figuring out the differences with regard to CoreFX, especially if something changes in CoreFX or if the "tone of the project" is sufficiently different. Making the guideline look more like Orleans, whatever that is then and whoever writes something concrete then. :)

[veikkoeeva]

There's really good discussion over at F# GH, so I link that fits the theme of this one. It links to wider CoreCLR discussion on the same subject. Let's be part of the family and at least listen to it, perhaps we get ideas (or just have our thoughts better in sync). :)

[veikkoeeva]

Spotted from @yevhen's Twitter stream: Ten Rules for Good Code.

[ReubenBond]

Our existing guidelines seem to be sufficient. We have our government-mandated code-of-conduct as well as our "contributing" docs page: https://dotnet.github.io/orleans/docs/resources/contributing.html

If we determine that we need something more, we can open a new issue, potentially referencing this.