From b7229280586e8694e3c1b1780eb96f117001ffbe Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Tue, 29 Aug 2023 18:07:25 +0200 Subject: [PATCH 1/4] Update CONTRIBUTING.md --- CONTRIBUTING.md | 88 ++++++++++++++++++++++++++++++------------------- 1 file changed, 54 insertions(+), 34 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cb5a7d7897a..b83507fbe6d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,57 +1,77 @@ # Contributing to Avalonia -## Before You Start +PRs are always welcomed from everyone. Following this guide will help us get your PR reviewed and merged as quickly as possible. -Drop into our [telegram group](https://t.me/Avalonia) or [gitter chat room](https://gitter.im/AvaloniaUI/Avalonia) and let us know what you're thinking of doing. We might be able to give you guidance or let you know if someone else is already working on the feature. +For this guide we're going to split PRs into two types: bug fixes and features; the requirements for each are slightly different. -## Style +## Bug Fixes -The codebase uses [.net core](https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/coding-style.md) coding style. +A bug fix will ideally be accompanied by tests. There are a few types of tests: -Try to keep lines of code around 100 characters in length or less, though this is not a hard limit. -If you're a few characters over then don't worry too much. +- Unit tests are for issues that aren't related to platform features. These tests are located in the `tests` directly, categorised by the assembly which they're testing. +- Integration tests are for issues that are related to platform features (for example fixing a bug with Window resizing). These tests are located in the `tests/Avalonia.IntegrationTests.Appium` directory. Integration tests should be run on Windows and macOS. See the readme in that directory for more information +- Render tests are for issues with rendering. These tests are located in `tests/Avalonia.RenderTests` with separate project files for Skia and Direct2D. The Direct2D backend is currently mostly unmaintained so render tests that just run on Skia are acceptable -**DO NOT USE #REGIONS** full stop. +It's not always feasible to accompany a bug fix with a test, but doing so will speed up the review process. -## Pull requests +The commits in a bug fix PR **should follow this pattern**: -A single pull request should be submitted for each change. If you're making more than one change, -please submit separate pull requests for each change for easy review. Rebase your changes to make -sense, so a history that looks like: +- A commit with a failing unit test; followed by +- A commit that fixes the issues -* Add class A -* Feature A didn't set Foo when Bar was set -* Fix spacing -* Add class B -* Sort using statements +In this way the reviewer can check out the commit with the failing test and confirm the problem. One this is confirmed, they can confirm the fix. -Should be rebased to read: +## Features -* Add class A -* Add class B +**Features should be discussed with the core team before opening a PR.** Please open an issue to discuss the feature before starting work, to ensure that the core team are onboard. -Again, this makes review much easier. +Features should always include unit tests or integration tests where possible. -Please try not to submit pull requests that don't add new features (e.g. moving stuff around) -unless you see something that is obviously wrong or that could be written in a more terse or -idiomatic style. It takes time to review each pull request - time that I'd prefer to spend writing -new features! +> One exception to this is features related to DevTools which has no tests currently -Prefer terseness to verbosity but don't try to be too clever. +Features that introduce new controls should consider the following: -## Tests +- Ideally the control should be exposed to the operating system's automation/accessibility APIs by writing an `AutomationPeer` +- If the control introduces any functionality which is difficult to unit test, an integration test should be written -There are two types of tests currently in the codebase; unit tests and render tests. +## General Guidance -Unit tests should be contained in a class name that mirrors the class being tested with the suffix --Tests, e.g. +## PR description - Avalonia.Controls.UnitTests.Presenters.TextPresenterTests +- The PR template contains sections to fill in. These are discretionary and are intended to provide guidance rather than being prescritive: feel free to delete sections that do not apply, or add additional sections +- **Please** provide a good description of the PR. Not doing so **will** delay review of the PR at a minimum, or may cause it to be closed. If English isn't your first language, consider using ChatGPT or another tool to write the description +- Link any fixed issues with a `Fixes #1234` comment -Where Avalonia.Controls.UnitTests is the name of the project. +## Breaking changes -Unit test methods should be named in a sentence style, separated by underscores, that describes in -English what the test is testing, e.g. +- During a major release cycle, source or binary breaking changes may not be introduced to the codebase: this is checked by an automated tool and will cause CI to fail +- If something needs addressing in the next major release, you can leave a `TODOXX:` comment, where `XX` is the version number of the next major release, e.g. `TODO12:` +- Carefully consider behavioral breaking changes and point them out in the PR description + +### Commits + +In addition to the guidance in the `Bug Fixes` section, please follow these guidelines: + +- Rebase your changes to remove extraneous commits. Ideally the commit history should tell a clean story of how the PR was implemented (even though the process was probably not clean!) +- Provide meaningful commit comments +- **Do not** change code unrelated to the bug fix/feature +- **Do not** introduce spurious formatting or whitespace changes + +While it's tempting to fix style issues you encounter, don't do it: + +- It causes the reviewer to get distracted by unrelated changes +- It makes finding the cause of any later issue more difficult (blame/bisect is made more difficult) +- As the code churns, style issues will be resolved anyway + +### Style + +- The codebase uses [.net core](https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/coding-style.md) coding style. +- Try to keep lines of code around 120 characters in length or less, though this is not a hard limit. If you're a few characters over then don't worry too much. +- Public methods should have XML documentation +- Prefer terseness to verbosity but don't try to be too clever. +- **DO NOT USE #REGIONS** full stop + +Tests do not follow the usual method naming convention. Instead they should be named in a sentence style, separated by underscores, that describes in English what the test is testing, e.g. ```csharp void Calling_Foo_Should_Increment_Bar() @@ -66,4 +86,4 @@ Render tests should describe what the produced image is: ## Code of Conduct This project has adopted the code of conduct defined by the Contributor Covenant to clarify expected behavior in our community. -For more information see the [Contributor Covenant Code of Conduct](https://dotnetfoundation.org/code-of-conduct) +For more information see the [Contributor Covenant Code of Conduct](https://dotnetfoundation.org/code-of-conduct) \ No newline at end of file From abc31ee724274f49f236c9320da575ae4cc39d45 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 13 Sep 2023 09:39:19 +0200 Subject: [PATCH 2/4] Tweak contributing. --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b83507fbe6d..ac6e9fc33ad 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,7 +50,7 @@ Features that introduce new controls should consider the following: ### Commits -In addition to the guidance in the `Bug Fixes` section, please follow these guidelines: +In addition to the guidance in the `Bug Fixes` section, following these guidelines may help to get your PR reviewed in a timely manner: - Rebase your changes to remove extraneous commits. Ideally the commit history should tell a clean story of how the PR was implemented (even though the process was probably not clean!) - Provide meaningful commit comments From 17db562f0bf47254f85a443e0f16236dff23f827 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 13 Sep 2023 09:39:29 +0200 Subject: [PATCH 3/4] Add link to contributing guide. --- .github/PULL_REQUEST_TEMPLATE.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 2f63750cdc5..3dd8361381b 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,3 +1,5 @@ + + ## What does the pull request do? From 34b0348ac31dbd59f7d751ed17af92e2347f6c61 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 13 Sep 2023 09:41:12 +0200 Subject: [PATCH 4/4] Added example "good" PR description. --- CONTRIBUTING.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ac6e9fc33ad..d3ed95005d9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -39,7 +39,7 @@ Features that introduce new controls should consider the following: ## PR description - The PR template contains sections to fill in. These are discretionary and are intended to provide guidance rather than being prescritive: feel free to delete sections that do not apply, or add additional sections -- **Please** provide a good description of the PR. Not doing so **will** delay review of the PR at a minimum, or may cause it to be closed. If English isn't your first language, consider using ChatGPT or another tool to write the description +- **Please** provide a good description of the PR. Not doing so **will** delay review of the PR at a minimum, or may cause it to be closed. If English isn't your first language, consider using ChatGPT or another tool to write the description. If you're looking for a good example of a PR description see https://github.com/AvaloniaUI/Avalonia/pull/12765 for example. - Link any fixed issues with a `Fixes #1234` comment ## Breaking changes @@ -63,6 +63,8 @@ While it's tempting to fix style issues you encounter, don't do it: - It makes finding the cause of any later issue more difficult (blame/bisect is made more difficult) - As the code churns, style issues will be resolved anyway +Separate PRs for style issues may be accepted if agreed with the core team in advance. + ### Style - The codebase uses [.net core](https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/coding-style.md) coding style.