Contributing guideline

[4rgon4ut]

This issue created to discuss contributing guideline details.

I created raw example adapted from template that I found good: https://gist.github.com/4rgon4ut/ae068b84a50c1b8ae66cd127337c1b9a

I have several topics to discuss(@bobbinth):

1. Do we actually need contributing guideline on current stage of the project?

2. Branching. As I see project have active branch named next and we also have gitbook separated branch, I would like to see your thoughts about which details should be represented.

3. Versioning. Looks like semver, but would be nice to clarify.

4. Any other details that should/shouldn't be represented, and guideline structure.

And the last, should I propose raw version to start discussion in proposal page or maybe create discussion in related section?(I mean link to the gist in issue do not looks like best way of doing it)

[bobbinth]

@4rgon4ut - thank you for bringing this up! @grjte will probably have more comments on this, but a few thoughts from my end:

1. Yes, I think this would be great to have.
2. Yes, next is intended to be the branch for developing next versions (v0.2 for now, and when v0.2 is released, next will be for v0.3 development). There still could be fixes against the main branch, but I don't foresee many of these happening, at least in the near future. a. Regarding gh-pages branch, I'm actually not sure yet what's the best way for managing going forward. Right now, the docs are coming from the next branch, but maybe they should come from the main branch in the future.
3. Yes, we use semver.

[grjte]

Hey @4rgon4ut I'm really happy to see this! It would be a great addition and I really like what you've put together. There are a couple things it would be good to discuss here and then after that maybe you can run it through google docs or similar for a quick grammar/spellcheck then open a PR where we can refine the rest. (I have some smaller comments I'd put inline)

I know we don't want to have a high barrier to entry, but I think some guidance is good.

1. There are certain things that we do consistently, and new contributors might appreciate being able to find about these things easily (like semantic commits or our documentation conventions).
2. Exposure to good practices (or at least opinionated ones) is beneficial for people who may not have had the opportunity to work in formal engineering environments, and we can link to explainer resources (as is done in the gist sample).

Branch names

Branch name should be a short issue/feature decription in snake case. For example, if issue title is Fix functionality X in component Y than branch name shuld be smth like: fix_x_in_y.

1. Bobbin and I both seem to use dashes, so maybe it would be better to specify using hyphens as separators, since that's what we've been doing.
2. If we're going to standardize this, we could consider prefixing by the issue number, e.g. 300-fix-x-in-y. Do either of you have an opinion on this?

Commit messages

I think a clean & descriptive git log is really important, so I'd like to address this at least at a basic level.

Ideally, I'd like people to:

• rebase from next before submitting a PR in case there have been changes. In other words - we'd like to avoid merge commits.
• use descriptive commit messages, preferably following the convention we've been using for semantic commit messages.
• rebase or squash commits when making changes in response to a code review on a PR (some projects object to this, so people may find it helpful to know our preference)

@bobbinth if you think asking people to rebase raises the barrier to entry too much, we could leave it out, or we could put it in (with link to a tutorial) but not be big sticklers about it during review. I would class this under point number 2 from above :)

Documentation and Code Style

There are a couple of things that we do which aren't written down anywhere which people usually learn at some point during reviews. It might be easier to include them here.

• For documentation in the codebase, we follow the rustdoc convention with no more than 100 characters per line.
• For code sections, we use code separators of 100 characters like the following:

// CODE SECTION HEADER
// ================================================================================================

[grjte]

I'd love to hear thoughts from other contributors on which of the things above would have been appreciated vs. discouraged you from contributing @tohrnii @0xKanekiKen and all others :)

[grjte]

Regarding gh-pages branch, I'm actually not sure yet what's the best way for managing going forward. Right now, the docs are coming from the next branch, but maybe they should come from the main branch in the future.

I think docs changes have been happening side by side with design changes we've been making, so it has been organic to have them on the next branch. I don't think forcing docs to happen elsewhere would have made sense for most of the cases in the past few months. I would keep doing docs on the next branch at least until the 0.3 release and then maybe reassess

[4rgon4ut]

1. Bobbin and I both seem to use dashes, so maybe it would be better to specify using hyphens as separators, since that's what we've been doing.
2. If we're going to standardize this, we could consider prefixing by the issue number, e.g. 300-fix-x-in-y. Do either of you have an opinion on this?

So we finalize on dashes as separators, if no one has any objections.

I'm personally think issue number in branch naming is also good and can save some time. However, we can standardize PR template so that PR description will contain issue number and link. Also would be nice to create PR/issue templates and add references to guide.

---

use descriptive commit messages, preferably following the convention we've been using for semantic commit messages.

Seems like good practice too but this requirement may lead to constantly having to remind people of this during reviews untill all project become consistent with this convention.

[grjte]

However, we can standardize PR template so that PR description will contain issue number and link. Also would be nice to create PR/issue templates and add references to guide.

Yep, I agree with this! Do you have something in mind already or templates you like?

Seems like good practice too but this requirement may lead to constantly having to remind people of this during reviews untill all project become consistent with this convention.

Yes, any conventions will lead to an increase in requesting that people check the contributor guidelines, so I think we'll see a similar issue with branch naming and doc line length. That said, I think we'd be ok with letting some things slide through for first-time contributors since we don't want to discourage anyone, and those things would probably be branch names, semantic commits, merging from next (instead of rebasing), and doc line length. We could point it out for repeat contributors, if needed. (This is basically what we do now, but it would be better because it's more accessible.) With semantic commits - currently most of the full members of the team are doing it but we're not enforcing it generally, so I think it would be good to make a decision & establish consistency at least among all regular contributors

We'll want to hear @bobbinth's thoughts on this though

[bobbinth]

I agree that pretty much all of this should be "soft requirements" for first time contributors. For example, if someone uses underscores instead of dashes in their first PR, I wouldn't necessarily call them out on that. For repeat contributors we can ask for stricter adherence to the guidelines, but also probably in a gradual manner.

A few specific comments:

• Yes, let's use dashes instead of underscores in branch names.
• I probably wouldn't require prefixing branch names with issue numbers as sometimes the scope of the PR changes while working on it. But this is not a strong opinion on my part.
• I'd probably favor keeping the PR template as light as possible as not to overwhelm people.
• I do think we should state that we follow semantic commit naming for commits.

[0xkanekiken]

I'd love to hear thoughts from other contributors on which of the things above would have been appreciated vs. discouraged you from contributing @tohrnii @0xKanekiKen and all others :)

When I started contributing, I didn't use to follow all the conventions that we have described above. But I think these things became obvious to me with time by inferring from the existing code base/commits. And TBH, none of the above conventions would take a substantial time/effort on the contributor side. I second @bobbinth's view that pretty much all of this should be "soft requirements" for the first-time contributor. I'd,though, likely not put "Writing Documentation" in the soft requirement list; we should request the contributor to adhere to our documentation guidelines.

[4rgon4ut]

@bobbinth @grjte Thanks for your thoughts. I added some points to the guide(on which consensus has been reached). There are also things in which we need to come to unity:

1. Prefixing branch names with issue number.

2. Rebase from next before submitting a PR.

As I understand, it is not "flow" disagreement, but rather trying to find right tradeoff between shortness(to not overflow newcommers) and descriptiveness(as all team members agree with this practise(?)). I thing that main part of contributors guide should be detailed enough. Also, we can add TL;DR section with ultra-short example-based soft requirements list to follow such scheme: Newcommer goes to guide and reading TL;DR section(with minimal base requirements). If one decides become repeat contributor later, he will return and read full guide by himself or will be redirected by team member on review process.(I personally think that full guide is not so long to read even with rebasing explanation, but such scheme may work for some people)

---

About Issue/PR templates:

- Issues

I found some ready to use templates but most of them looks overloaded. I do not think we need issues template on current stage because most of them too narrow-minded, and we have a lot of discussions and design thoughts in issues and we will face classification problem along with template mismatch issues.

- PR

I agree with @bobbinth that PR template should be light. It may be good to move "Pre-PR checklist" section from contributors guide to PR template to get something like this:

## Describe your changes

## Issue ticket number and link

## Checklist before requesting a review
- [ ] Repo forked and branch created from `next` according to naming convention.
- [ ] Commit messages and codestyle follow conventions.
- [ ] Tests added for new functionality.
- [ ] Documentation/comments updated according to changes.
- [ ] Tests passed.

Ofc include links to related sections of contributors guide in template.

[grjte]

@4rgon4ut Great! Here are my thoughts on the above.

1. Prefixing branch names with issue number.

Let's not do this. We'll just keep it simple with the existing kebab-case branch naming as you've described it.

2. Rebase from next before submitting a PR.

I think we should ask people to rebase. There seems to be agreement from those contributors who have weighed in (you, @0xKanekiKen, and myself) that this is not too much to ask and would not have discouraged us initially. And as stated, we can let it be a "soft" requirement, so we won't make first-timers fix it if they don't get it right. (Although we don't need to state this explicitly anywhere. We'll just keep it in mind within the team)

we can add TL;DR section with ultra-short example-based soft requirements list to follow such scheme

This is a good idea, but actually I don't think it's necessary right now. The new doc version is still pretty short & manageable, and I think the lightweight PR template you described below does the job of providing a TL;DR for people who don't read the whole doc.

I do not think we need issues template on current stage because most of them too narrow-minded, and we have a lot of discussions and design thoughts in issues and we will face classification problem along with template mismatch issues.

Yep, good points, I agree!

It may be good to move "Pre-PR checklist" section from contributors guide to PR template

I like this - I think the PR template you suggested will be helpful without being overwhelming. I would just make a couple minor changes:

1. include the step to rebase from next
2. Maybe add a step for clippy and rustfmt passed? If they aren't passing it'll just fail in the PR run whenever one of us starts it, and it might be a bit frustrating to diligently go through the checklist & still have that happen.
3. I think we could still keep the "Pre-PR checklist" in the contributor doc. There's no harm in having it in both places, since it's useful & basically acts as a TL;DR.

[bobbinth]

I agree with pretty much everything what @grjte wrote above. One question:

How does having a PR checklist affect number of tasks that Github shows in the summary of PRs? We frequently use checklists to track the progress within PRs, and it would probably create too much noise if each PR has 5 - 6 tasks listed by default.

[grjte]

@bobbinth what about having it as a bullet point list instead of a task list?

[bobbinth]

I think having bullet points should work. Also, I wonder if for the template:

• Instead of Issue ticket number and link section we should just have a bullet point saying something like Relevant issues are linked in the description.
• Add the link to "Commit messages and codestyle follow conventions." to the the actual doc.
• Get rid of Tests passed. bullet point as I think this is usually implied.

Basically, it could look like this:

## Describe your changes

## Checklist before requesting a review
- Repo forked and branch created from `next` according to naming convention.
- Commit messages and codestyle follow [conventions](./CONTRIBUTING.md).
- Relevant issues are linked in the PR description.
- Tests added for new functionality.
- Documentation/comments updated according to changes.

These are not strong preferences from my end - just suggestions. So, I could go either way.

[4rgon4ut]

@bobbinth @grjte I'm added rebase and squashing points to guide and fixed some typos. Would be great to know your thoughts about proposing version. Also, added clippy and rustfmt passing point to Pre-PR checklist. As you can see I failed with proposing guide through community standards interface because proposing create PR to main with default branch name patch-1 which is inconsistent with naming convention. Should I propose through common PR flow?

---

About PR template

As I see we are all agreed on using bullet point instead of checkboxes. Another thing to clarify: we are going to store PR template(and possible future templates) in .github directory?

[bobbinth]

Closed by #365 and 62e5889b503bafe601c378c95608969975f55aeb