Proposed revisions for contributors guide

[markcmiller86]

@epourmal am submitting a PR to you hear with some ideas for revision to your contributor's guide.

Several things came up for me as I read it and hopefully this GitHub conversatiion will be a useful venue in discussin some of them. Please take my suggestions here as offered in the spirit of trying to further this conversation.

• As much as possible I added links to supporting resources
• I noticed you used some embedded HTML for anchors. You don't need to do that. GitHub Flavored Markdown (GFM) defines anchors, implicitly, for level 1 through level 6 headings.
• Do you want to give contributors an idea of the maximum size of any PR that is acceptable? I think this might be a good idea and seems to me to be consistent with other projects. Larger efforts need to be decomposed into smaller PRs. I think this could be really important. I proposed 50 files and 500 lines of code.
• You referred to a file, issue_template.md that doesn't exist. You can create an issue template for GitHub issues that will come up any time a user wants to file an issue. That template can include a checklist. I can explain further if needed.
• You explain which branches different work should go into but don't explain who is responsible for ensuring that work also gets migrated to other branches. Who is responsible, THG or contributor, for migrating a bugfix from develop to one or more release branches? IMHO...the onis for this should be on THG, not the contributor. But, this question of which branch some work should go into may require upfront discussion with the contributor ahead of the work. If so, the guidelines should probably just say that.
• How long will THG allow a PR to sit before review is completed? That depends on PR size BTW. I think contributors need some idea of how long this might be. You don't need a crystal ball. But, expectations do need to be set.
• In the criteria, you mention purpose. I think part of purpose involves ensuring scope is well constrained. That also plays into PR size question.
• If performance arguments are to be given for purpose, then the performance data supporting them needs to be included. A contributor's SWAG that s/he thinks "performance is better with these changes" should not be sufficient. Performance data from a single test or platform is not sufficient, either.
• I think it makes sense that users describe the workflow(s) where the proposed changes are beneficial. Maybe they provide working use-case examples too?
• You mention importance of documentation in criteria too but I think that is actually quite a wide range of requirements there. I tried to propose verbiage for some of that. I think good examples here might be important to help set contributor's thinking caps.
• You mention the importance of backward compatability and that too has a wide range of requirements...file format, API, ABI, machine independence, VFD and VOL independence, etc. I proposed some changes to speak to that too.

[epourmal]

@epourmal am submitting a PR to you hear with some ideas for revision to your contributor's guide.

Thank you! I am planning to review your suggestions on 3/19 with developers.

Several things came up for me as I read it and hopefully this GitHub conversatiion will be a useful venue in discussin some of them. Please take my suggestions here as offered in the spirit of trying to further this conversation.

* As much as possible I added links to supporting resources

Thank you! Some resources are missing and will be added as they become available (for example, backward/forward compatibility).

* I noticed you used some embedded HTML for anchors. You don't need to do that. GitHub Flavored Markdown (GFM) defines anchors, implicitly, for level 1 through level 6 headings.

Good to know!

* Do you want to give contributors an idea of the _maximum_ size of any PR that is acceptable? I think this might be a good idea and seems to me to be consistent with other projects. Larger efforts need to be decomposed into smaller PRs. I think this could be really important. I proposed 50 files and 500 lines of code.

Good question. I am not sure if it is always possible. I think we all agreed that clear purpose of PR and "one feature at a time" is a mitigation to the size of PR along with self-contained changes. It is a nightmare when changes are not localized and scattered all over the code.

* You referred to a file, `issue_template.md` that doesn't exist. You can create an issue template for GitHub issues that will come up any time a user wants to file an issue. That template can include a checklist. I can explain further if needed.

Thank you! It turned out we know how to set it up and will do it. My goal was to create a file so they can implement it. I've already removed the reference to the template file from the current version.

* You explain which branches different work should go into but don't explain _who_ is responsible for ensuring that work also gets migrated to other branches. Who is responsible, THG or contributor, for migrating a bugfix from `develop` to one or more release branches? IMHO...the onis for this should be on THG, not the contributor. But, this question of _which branch_ some work should go into may require upfront discussion with the contributor ahead of the work. If so, the guidelines should probably just say that.

We had long discussions about it and decided to say what is in the document (pretty vague).

We always discuss where the fix should go and there are several factors that affect the decision (complexity, differences in the code among different branches, relevance of PR, etc.) We have set up an internal process that looks like this: we triage PRs on a regular basis during engineering stand-ups each morning, we also review the progress on PRs every Friday at the engineering meeting. During triaging we agree if PR should go to other branches and we open a ticket for this task. Depending on complexity of PR we decide who does it and contact the author, because in some cases the author of PR will be the best to merge the code to other branches. So far it worked well with both internal and external contributors. I will add the description of the process to it would be clear that we may ask and guide a contributor.

* How long will THG allow a PR to sit before review is completed? That depends on PR size BTW. I think contributors need some idea of how long this might be. You don't need a crystal ball. But, expectations do need to be set.

We start reviewing as soon as PT comes. If it is a bug fix with a test, it usually goes in very quickly (in a week or less). Based on our few months of experience working with just two major external contributors that try to change library all over the places (compiler warnings, or new features) the ranges are from an hour to "forever". We spent on average 6-8 hours a day dealing with external contributions (reviewing and going back and forth and then addressing regression testing issues).

I think what is important that communications with the PR author start right away and a clear path to acceptance is developed.

* In the criteria, you mention purpose. I think part of purpose involves ensuring _scope_ is well constrained. That also plays into PR size question.

Right.

* If performance arguments are to be given for purpose, then the performance data supporting them needs to be included. A contributor's SWAG that s/he thinks "performance is better with these changes" should not be sufficient. Performance data from a single test or platform is not sufficient, either.

Agree. We are running performance benchmarks internally, so we will know if PR affects performance and we will revert it. We should provide contributors with benchmark set that they can easily run just on their platform and compare results before and after.

I will explain about reverting PRs if internal performance testing shows degradation.

* I think it makes sense that users describe the workflow(s) where the proposed changes are beneficial. Maybe they provide working use-case examples too?

Yes and this should be in the ticket for PR.

* You mention importance of _documentation_ in criteria too but I think that is actually quite a wide range of requirements there. I tried to propose verbiage for some of that. I think good examples here might be important to help set contributor's thinking caps.

Yes, it is a quite range and depends on PR. I tried to keep the document short and emphasize importance of communications.
Your code example is good, and I start laughing because our code has a lot of such comments :-)

* You mention the importance of backward compatability and that too has a wide range of requirements...file format, API, ABI, machine independence, VFD and VOL independence, etc. I proposed some changes to speak to that too.

Thank you! And I need to dig out our docs on backward and forward compatibility. There is still a lot of work to do, but at least we started!