add PR guideline, update CONTRIBUTING.md, move spellcheck #5390

drahnr · 2022-04-25T15:46:44Z

Formalize a rough outline of what contributors should look out for. This is the first attempt to do so and provide some convenience, especially around referencing companion PRs, which I tend to forget every single time I need them.

.github/PULL_REQUEST_TEMPLATE.md

ordian · 2022-04-25T15:50:22Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Workspace compiles `cargo build --workspace`?
+- [ ] Workspace tests `cargo test --workspace`?
+- [ ] Applied `cargo +nightly fmt` with the version from CI?
+- [ ] Ran `cargo spellcheck` without any fallout? If any, update `.spellcheck/lingua.dic`.


Is the full command

spellcheck check -cfg=.spellcheck/spellcheck.toml --checkers hunspell

or is list-files required first for optimization? Would be nice to include instructions how to update the dictionary exactly or a link to that.

That's on my todo to print those instructions from cargo-spellcheck directly, covered by drahnr/cargo-spellcheck#193 and planned to be done thisweek™

.github/PULL_REQUEST_TEMPLATE.md

ordian · 2022-04-25T15:57:03Z

.github/PULL_REQUEST_TEMPLATE.md

+
+- [ ] Labels applied correctly? See the [contributing guidlines](../CONTRIBUTING.md).
+- [ ] Workspace compiles `cargo build --workspace`?
+- [ ] Workspace tests `cargo test --workspace`?


That might be to heavy for some external contributors' laptops to handle ;)

Good point 👍

it's being executed by CI, why bother doing it locally?

Because the turnaround time is significantly faster doing it locally iff your local machine can handle it :)

Added additional indentation to make sure it's optional and stated that explicitly.

alvicsam · 2022-04-25T16:03:10Z

.gitlab-ci.yml

@@ -273,7 +273,7 @@ spellcheck:
    - git fetch origin +${CI_DEFAULT_BRANCH}:${CI_DEFAULT_BRANCH}
    - echo "___Spellcheck is going to check your diff___"
    - cargo spellcheck list-files -vvv $(git diff --diff-filter=AM --name-only $(git merge-base ${CI_COMMIT_SHA} ${CI_DEFAULT_BRANCH} -- :^bridges))
-    - time cargo spellcheck check -vvv --cfg=scripts/gitlab/spellcheck.toml --checkers hunspell --code 1
+    - time cargo spellcheck check -vvv --cfg=.spellcheck/spellcheck.toml --checkers hunspell --code 1


Is it necessary to move this file to the .spellcheck folder? Recently we unified the structure of CI files in all major repos (https://github.com/paritytech/ci_cd/issues/313) and would like to keep all files in one place.

Yes, I was opposing the change to move them out of there from the get go, since that's the default discovery location of cargo-spellcheck and avoids yet another arg required.

arg is not that bad, but what if someone wants several config files for the different crates, dirs, and languages?

btw you still use the extra arg here

Yes, I use the extra arg here for explicitness. But for the average user I'd not want that. If the file does not reside in the default lookup dir, the contributor sees an excessive amount of detected errors, that are covered by lingua.dic.

Co-authored-by: Andronik <[email protected]>

bkchr · 2022-04-25T18:16:22Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Workspace compiles `cargo check --workspace --tests`?
+- [ ] Workspace tests `cargo test --workspace`?
+- [ ] Applied `cargo +nightly fmt` with the version from CI?
+- [ ] Ran `cargo spellcheck` without any fallout? If any, update `.spellcheck/lingua.dic`.


All that is ensured by the CI, not sure why this needs to be mentioned here.

I think, guidance on how to please the CI is a generally good and nice thing to state in a PR. The above might indeed be too verbose, but i.e. cargo-spellcheck and cargo fmt are something that wasn't covered before, but we only point to a style guide that is not so accurate anymore.

I think, guidance on how to please the CI is a generally good and nice thing to state in a PR. The above might indeed be too verbose, but i.e. cargo-spellcheck and cargo fmt are something that wasn't covered before, but we only point to a style guide that is not so accurate anymore.

I tend to agree this is a bit redundant and just a checklist to fill by hand once you contribute a few times, but for first time contributors, it's really good to have this information ahead of time and not wonder through the CI errors to figure out what can be wrong for such common issues.

We can remove the checklist property and just make it a list of things to cover.

Can we not just put this into the CONTRIBUTING.md?

Sure, works as well

sandreim · 2022-04-26T08:57:34Z

CONTRIBUTING.md

@@ -8,7 +8,10 @@ There are a few basic ground-rules for contributors (including the maintainer(s)
 - **Non-master branches**, prefixed with a short name moniker (e.g. `gav-my-feature`) must be used for ongoing work.
 - **All modifications** must be made in a **pull-request** to solicit feedback from other contributors.
 - A pull-request _must not be merged until CI_ has finished successfully.
- Contributors should adhere to the [house coding style](https://github.com/paritytech/polkadot/wiki/Style-Guide).
+- Contributors should apply `cargo +nightly fmt` with the version used in CI.


Suggested change

- Contributors should apply `cargo +nightly fmt` with the version used in CI.

- Contributors must apply `cargo +nightly fmt` with the version used in CI.

Suggested change

- Contributors should apply `cargo +nightly fmt` with the version used in CI.

- Contributors should apply `cargo +nightly fmt` with the version used in CI (up-to-date CI image: `paritytech/ci-linux:production`).

but still, no need in this step as it's done by CI anyway

I was under the impression we could use the rust-toolchain file for specifying the the supported toolchain rather than pulling in the whole container env, but it seems it only supports a single rust version reference and being unspecific regarding the tool rust-lang/rustup#1172

CONTRIBUTING.md

TriplEight · 2022-04-26T09:19:46Z

.github/PULL_REQUEST_TEMPLATE.md

+
+- [ ] Labels applied correctly? See the [contributing guidlines](../CONTRIBUTING.md).
+- [ ] Workspace compiles `cargo build --workspace`?
+- [ ] Workspace tests `cargo test --workspace`?


it's being executed by CI, why bother doing it locally?

TriplEight · 2022-04-26T09:20:17Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Workspace compiles `cargo check --workspace --tests`?
+- [ ] Workspace tests `cargo test --workspace`?
+- [ ] Applied `cargo +nightly fmt` with the version from CI?
+- [ ] Ran `cargo spellcheck` without any fallout? If any, update `.spellcheck/lingua.dic`.


TriplEight · 2022-04-26T09:21:26Z

.gitlab-ci.yml

@@ -273,7 +273,7 @@ spellcheck:
    - git fetch origin +${CI_DEFAULT_BRANCH}:${CI_DEFAULT_BRANCH}
    - echo "___Spellcheck is going to check your diff___"
    - cargo spellcheck list-files -vvv $(git diff --diff-filter=AM --name-only $(git merge-base ${CI_COMMIT_SHA} ${CI_DEFAULT_BRANCH} -- :^bridges))
-    - time cargo spellcheck check -vvv --cfg=scripts/gitlab/spellcheck.toml --checkers hunspell --code 1
+    - time cargo spellcheck check -vvv --cfg=.spellcheck/spellcheck.toml --checkers hunspell --code 1


btw you still use the extra arg here

TriplEight · 2022-04-26T09:22:06Z

CONTRIBUTING.md

@@ -8,7 +8,10 @@ There are a few basic ground-rules for contributors (including the maintainer(s)
 - **Non-master branches**, prefixed with a short name moniker (e.g. `gav-my-feature`) must be used for ongoing work.
 - **All modifications** must be made in a **pull-request** to solicit feedback from other contributors.
 - A pull-request _must not be merged until CI_ has finished successfully.
- Contributors should adhere to the [house coding style](https://github.com/paritytech/polkadot/wiki/Style-Guide).
+- Contributors should apply `cargo +nightly fmt` with the version used in CI.


but still, no need in this step as it's done by CI anyway

CONTRIBUTING.md

Co-authored-by: Andrei Sandu <[email protected]>

drahnr · 2022-04-26T14:51:01Z

Just to clarify the intent of this PR:

It's supposed to give both people being onboarded as well as external contributors a minimal guide how to pass the CI hurdle.

That said, we might shift the more detailed instructions to the CONTRIBUTING.md, my point being, that it must be as easy to discover as possible :)

pepyakin · 2022-04-27T16:10:56Z

.github/PULL_REQUEST_TEMPLATE.md

+
+## Checklist
+
+- [ ] Labels applied correctly? See the [contributing guidlines](../CONTRIBUTING.md).


External contributors cannot apply labels.

pepyakin · 2022-04-27T16:14:58Z

.github/PULL_REQUEST_TEMPLATE.md

+<!--
+## Companions:
+
+Must be last!


Why though? And is the contributor supposed remove the comment? Otherwise, it won't be last due to the -->

pepyakin · 2022-04-27T16:16:46Z

.github/PULL_REQUEST_TEMPLATE.md

+
+<!--
+Apply labels!
+-->


This looks a bit too much imperative... also I feel it might be not clear to a new contributor what exactly he should do here.

The item in the checklist helps, but why do we need this comment here then?

pepyakin · 2022-04-27T16:22:10Z

.github/PULL_REQUEST_TEMPLATE.md

+
+Fixes #
+Ref #
+-->


I feel that an external contributor may not be fully clear on what this is. I feel that this should be full sentences. Maybe something along those lines:

"Hello and thank you for the pull request.

Please leave a crisp summary of the issue that you are solving with the issues linked that describe the issue itself".

I don't think we should strive for brevity here. The reason is, a user to developer will probably skip the static prompt part. I can guarantee that this will be an automatic action for most people. They won't even notice it after a while.

On the other hand, a newcomer may appreciate if you write in the expanded form right away.

pepyakin · 2022-04-27T16:25:16Z

CONTRIBUTING.md

+### Helping out
+
+We use labels to manage PRs and issues and communicate state of a PR. Please familiarize yourself with them. Furthermore we are organizing issues in milestones. Best way to get started is to a pick a ticket from the current milestone tagged easy or medium and get going or mentor and get in contact with the mentor offering their support on that larger task.
+Issues


Suggested change

Issues

Issues

pepyakin · 2022-04-27T16:26:03Z

CONTRIBUTING.md

@@ -33,6 +36,21 @@ When reviewing a pull request, the end-goal is to suggest useful changes to the
 - There exists a somewhat cleaner/better/faster way of accomplishing the same feature/fix.
 - It does not fit well with some other contributors' longer-term vision for the project.

+### Helping out
+
+We use labels to manage PRs and issues and communicate state of a PR. Please familiarize yourself with them. Furthermore we are organizing issues in milestones. Best way to get started is to a pick a ticket from the current milestone tagged easy or medium and get going or mentor and get in contact with the mentor offering their support on that larger task.


We could also provide quick links to those task queries to reduce friction a bit for a potential contributor.

pepyakin · 2022-04-27T16:29:19Z

CONTRIBUTING.md

+We use labels to manage PRs and issues and communicate state of a PR. Please familiarize yourself with them. Furthermore we are organizing issues in milestones. Best way to get started is to a pick a ticket from the current milestone tagged easy or medium and get going or mentor and get in contact with the mentor offering their support on that larger task.
+Issues
+
+Please label issues with the following labels:


Suggested change

Please label issues with the following labels:

Issues must be labelled with the following labels:

I am suggesting this to not confuse the readers. Right now it reads as if we are asking the new contributors to help label issues.

Ideally, we don't accept this change but just move this guidance elsewhere.

pepyakin · 2022-04-27T16:31:14Z

ADR.md

+## Consequences
+
+<!-- What becomes easier or more difficult to do because of this change? -->
+```


Is this something that was discussed somewhere already?

Discussed in what regard? We need a form of documenting far reaching implementation choices.

This sounds like quite a significant change for the existing workflow. Thus I would expect a discussion to happen regarding it. At the same time, I have missed quite some of the parachains calls. So I thought maybe I could catch up with that or get a gist of that discussion somewhere.

This repo is also not just about parachains. I don't have seen any discussion about this change somewhere.

The intent was mostly to guide people, and the ADR is something that's an approach to document things and reduce the tribal knowledge on why things are as they are.

drahnr · 2022-05-16T06:51:41Z

Closing, partially since the spellcheck changes are not needed (forgot the reference from the manfiest is also there) and the remaining topics will be split into individual items.

drahnr added 3 commits April 25, 2022 17:45

move cargo-spellcheck related files back to the default pickup path

4ab0202

update CONTRIBUTING.md

f7303bb

add PR template

ff4ea87

drahnr requested review from a team and chevdor as code owners April 25, 2022 15:46

github-actions bot added the A0-please_review Pull request needs code review. label Apr 25, 2022

lint

e94efb6

ordian reviewed Apr 25, 2022

View reviewed changes

alvicsam reviewed Apr 25, 2022

View reviewed changes

Update .github/PULL_REQUEST_TEMPLATE.md

bdf3752

Co-authored-by: Andronik <[email protected]>

bkchr reviewed Apr 25, 2022

View reviewed changes

sandreim reviewed Apr 26, 2022

View reviewed changes

TriplEight suggested changes Apr 26, 2022

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

drahnr and others added 3 commits April 26, 2022 12:15

Update CONTRIBUTING.md

cea43ed

Co-authored-by: Andrei Sandu <[email protected]>

add ADR.md

0d5b720

make sure these commands are optional

d4263fe

pepyakin reviewed Apr 27, 2022

View reviewed changes

drahnr self-assigned this Apr 29, 2022

drahnr closed this May 16, 2022

bkchr deleted the bernhard-contrib branch May 16, 2022 07:39

	- Contributors should apply `cargo +nightly fmt` with the version used in CI.
	- Contributors must apply `cargo +nightly fmt` with the version used in CI.


		## Checklist

		- [ ] Labels applied correctly? See the [contributing guidlines](../CONTRIBUTING.md).

	Please label issues with the following labels:
	Issues must be labelled with the following labels:

add PR guideline, update CONTRIBUTING.md, move spellcheck #5390

add PR guideline, update CONTRIBUTING.md, move spellcheck #5390

Conversation

drahnr commented Apr 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreim Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr commented Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr May 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr commented May 16, 2022

drahnr commented Apr 25, 2022 •

edited

Loading

drahnr Apr 26, 2022 •

edited

Loading

drahnr Apr 26, 2022 •

edited

Loading

sandreim Apr 26, 2022 •

edited

Loading

drahnr commented Apr 26, 2022 •

edited

Loading

bkchr May 13, 2022 •

edited

Loading