Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CONTRIBUTING.md #12714

Merged
merged 4 commits into from
Sep 13, 2023
Merged

Update CONTRIBUTING.md #12714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b722928
Update CONTRIBUTING.md
grokys Aug 29, 2023
abc31ee
Tweak contributing.
grokys Sep 13, 2023
17db562
Add link to contributing guide.
grokys Sep 13, 2023
34b0348
Added example "good" PR description.
grokys Sep 13, 2023
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
2 changes: 2 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
<!--- See CONTRIBUTING.md for general guidelines on contributions -->

## What does the pull request do?
<!--- Give a bit of background on the PR here, together with links to with related issues etc. -->

Expand Down
90 changes: 56 additions & 34 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,57 +1,79 @@
# 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this change, it's just that the team is often very busy and therefore doesn't respond to discussions, I wouldn't want this to lead to fewer contributions in the long run.

Copy link
Contributor

Choose a reason for hiding this comment

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

True and not true. It also leads to frustration if a PR doesn't get merged in the end. So hard to get a good measure here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is unfortunate, but I prefer contributors not to waste days or weeks of time on something which isn't going to be accepted anyway. Of course no-one is stopping people opening undiscussed PRs, but with this guidance maybe they'll know what they're getting into.


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

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, 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
- **Do not** change code unrelated to the bug fix/feature
- **Do not** introduce spurious formatting or whitespace changes
Copy link
Contributor

@robloo robloo Sep 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I know we have differences of opinion here but I think this is too strict of a policy. It doesn't allow fixes to ever occur unless the entire file is replace. Code churn as you put it needs to happen.

IMO we need:

  1. An actual code formatting guide with enforced rules in an .editorconfig (has been needed for a while)
  2. Relax formatting changes like this so they are allowed if you notice something in the file you are already editing. Most projects allow fixes if you are already working in the file or its related to the feature you are working on.
  3. Clarify that formatting changes should only be of clear benefit: Modern syntax like get { return ... to get => ..., remove blank lines with spaces, add space after // in comment, etc. These are unarguable for the most part. New devs will sometimes open PRs changing things that are unnecessary but that can be discussed case-by-case. I don't think it should be prohibited outright.

Finally, I get the impression you are a purist with git history. Maintaining a clear and easy to read history is most important to you. I've found its often better to just let the code evolve. It's rare that the full git history needs to be investigated. And when it does need to be investigated it can still be done even with formatting changes. It just requires a little more work. Bottom line I rank the cost/benefit different than you and prioritize code quality over git history quality.

Copy link
Member

@kekekeks kekekeks Sep 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's all logical and stuff, but you aren't the one who has to go through the file history when diagnosing issues because some random change has polluted git blame. We are doing it quite often and it's a rather annoying procedure.
And unlike preparing a PR it will have to be done forever while the code exists.

Another problem is reverting PRs that caused regressions. If PR was merged several months ago and it had formatting changes, there is a huge chance that it won't get reverted cleanly because the affected random pieces of code were already changed by other PRs. e. g. it was pretty much impossible to revert a change made a while ago which was rather frustrating and have cost me several hours of my life.

So I'd rather ignore the PR completely than increase the maintenance burden because of somebody's ideas of beauty.

Copy link
Member

Choose a reason for hiding this comment

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

TL;DR: PR author is not the one who will have to maintain the code ad aeternam, so clean history is more important.

Copy link
Contributor

@robloo robloo Sep 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

because of somebody's ideas of beauty.

It's not beauty, it's code quality I'm advocating for. I think code quality is much more important than perfect git history. In fact I've never heard of someone who will refuse fixes because it will make the git history more complicated.

Again, I propose a few compromises;

  • Only focus on clear improvements (which can be agreed ahead of time with an editorconfig rule set)
  • Only allow formatting fixes for files already being edited for other reasons

You could also still allow formatting-only PRs that would fix stuff like this.

Several make commits without detailed review and a lot of formatting mistakes get made. I often see missed spaces after if for example in addition to what I stated above. It doesn't make sense not to allow fixes this. If you stand firm that these shouldn't be fixed I'll argue that we need to be much, much more careful during PR merges. In fact we should be running code formatting checks before allowing merge if you want to go this route.

maintain the code ad aeternam, so clean history is more important.

That logic doesn't seem to hold. The main reasons to go through the history are to find the original authors and ask them to take a look. Or to find the related PR's, issues and discussions to understand more background. As you already said, original authors come and go so there is a decreasing significance over time with git blame (significance approaches zero over project lifetime).

Concerning related PR's issues and discussions several projects provide a lot of comments and references in code to avoid having to dig through to find related issues and discussions. In the end PR is still useful to get to with git history though.

Edit:

Another thought: I don't think the idea of 'clean/perfect git history' meshes well with open source software development. Open source software development is inherently a bit more unstructured than a small team can manage. It is better to allow faster, more changes. Code churn will fix things over time and you shouldn't restrain it. The true power of community development is noticing the smaller issues too which seems to imply many PRs which will convolute git history anyway. (Most people will find the little things and what doesn't work, Few people will push forward new features).

Copy link
Contributor

@robloo robloo Sep 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

but you aren't the one who has to go through the file history when diagnosing issues because some random change has polluted git blame. We are doing it quite often and it's a rather annoying procedure.

Also, are you using git bisect for this? It can filter past a lot of commits quickly.

Copy link
Member Author

@grokys grokys Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

These guidelines are aimed at improving the speed in which we can review PRs. You may have noticed that we have an ever-growing list of open PRs and not enough time to review them.

Not following these guidelines doesn't mean we close your PR, but it does mean that it's less likely to get reviewed in a timely manner. Personally, my brain gets very distracted by unrelated changes so much that I'll move onto some other urgent job instead of finishing reviewing.

As a concrete example, take a look at one of @MrJul's PRs such as #12765 and see how quickly you can review it. There's a good description, you can run the commit with the test before the fix to verify the problem and there are no extraneous changes. There's a reason that most of his PRs get merged quickly; they're a joy to review.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, are you using git bisect for this? It can filter past a lot of commits quickly.

Yes, i use this almost daily and non-building or intermediate commits make it a terrible experience.


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

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.
- 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()
Expand All @@ -66,4 +88,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)