Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Description

<!--
A crisp summary
Fixes #
Ref #
-->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


<!--
Apply labels!
-->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


## Checklist

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External contributors cannot apply labels.

- [ ] Workspace compiles `cargo build --workspace`?
- [ ] Workspace tests `cargo test --workspace`?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@drahnr drahnr Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- [ ] Applied `cargo +nightly fmt` with the version from CI?
- [ ] Ran `cargo spellcheck` without any fallout? If any, update `.spellcheck/lingua.dic`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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™

- [ ] Fixed any fallout caused by these changes in `cumulus`?
- [ ] Referenced relevant companion pullrequests of `substrate` and `cumulus`?
- [ ] Does the change mandate an _architecture design record_ being committed?

<!--
## Companions:
Must be last!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

substrate companion: https://github.com/paritytech/substrate/pulls/
cumulus companion: https://github.com/paritytech/cumulus/pulls/
-->
4 changes: 1 addition & 3 deletions .gitlab-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw you still use the extra arg here

Copy link
Contributor Author

@drahnr drahnr Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$(git diff --diff-filter=AM --name-only $(git merge-base ${CI_COMMIT_SHA} ${CI_DEFAULT_BRANCH} -- :^bridges))
allow_failure: true

Expand Down Expand Up @@ -854,5 +854,3 @@ cancel-pipeline:
PROJECT_ID: "${CI_PROJECT_ID}"
PIPELINE_ID: "${CI_PIPELINE_ID}"
trigger: "parity/infrastructure/ci_cd/pipeline-stopper"


File renamed without changes.
20 changes: 19 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

- Synchronized changes must have annotations with the respective [companion PRs of substrate and cumulus](https://github.com/paritytech/substrate/blob/master/docs/CONTRIBUTING.adoc#updating-polkadot-as-well), tl;dr add the following lines at the _end_ of your PR description:
- `substrate companion: https://github.com/paritytech/substrate/pulls/#1234`
- `cumulus companion: https://github.com/paritytech/cumulus/pulls/#5678`

### Merging pull requests once CI is successful

Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Issues
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Issues
Issues


Please label issues with the following labels:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


I-* Issue severity and type. EXACTLY ONE REQUIRED.

P-* Issue priority. AT MOST ONE ALLOWED.

Q-* Issue difficulty. AT MOST ONE ALLOWED.

Z-* More general tags on the issue, denoting context and resolution.

## Releases

Declaring formal releases remains the prerogative of the project maintainer(s).
Expand Down