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.

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

docs: Add CONTRIBUTING.md #272

Merged
merged 10 commits into from
Aug 20, 2023
71 changes: 71 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# Contributing to AccessKit

Comment on lines +1 to +2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A welcoming message, perhaps including something about what types of contributions you are looking for / open to would be fitting to have here.

Felt like it would be disingenuous if I tried to write one as I'm not a regular maintainer.

## Making a Pull Request

### Cargo.lock File

AccessKit intentionally includes the `Cargo.lock` file in our git repository.
This is mainly due to our bindings to other languages.
Copy link
Contributor Author

@paul-hansen paul-hansen Aug 12, 2023

Choose a reason for hiding this comment

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

A better defense of why this is needed might be a good addition here. I don't know the specific reason myself so this was the best I had.


Usually you should not run cargo update when creating a pull request even when adding/changing a dependency.
Simply building the library should update the `Cargo.lock` file with the minimal changes needed.

> [!NOTE]
> This is not normal / best practice for most libraries.
> If you are writing your own library, your `Cargo.lock` file is not used when the library is included in another project.
> This means your CI will be testing with an out of date set of dependencies compared to your users, missing potential problems.
> See the [official documentation](https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries) for more information.

### CHANGELOG.md

Our CHANGELOG.md files are auto generated using [Release Please](https://github.com/googleapis/release-please) and should not be edited manually.

To control what is in the CHANGELOG.md for your change, use [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) when writing your commit messages.
paul-hansen marked this conversation as resolved.
Show resolved Hide resolved

#### Example Commit Messages
Taken from [Conventional Commits documentation](https://www.conventionalcommits.org/en/v1.0.0/#summary):
> feat: allow provided config object to extend other configs
>
> BREAKING CHANGE: `extends` key in config file is now used for extending other config files

> fix: prevent racing of requests
>
> Introduce a request id and a reference to latest request. Dismiss
incoming responses other than from latest request.
DataTriny marked this conversation as resolved.
Show resolved Hide resolved

### Testing Locally

We have some platform specific tests that are not run with `cargo test` from the project root by default.

1. Run test that work on all platforms
``` shell
cargo test
```
2. Run platform specific tests.

Run the appropriate command for your platform
``` shell
cargo test -p accesskit_windows
cargo test -p accesskit_unix
cargo test -p accesskit_macos
```
Not all platforms have tests at this time, but they may in the future.

> [!WARNING]
> **Windows**: Some tests may fail if the created window looses focus while testing. This can also happen when using the terminal built into your IDE. Try running them from your native terminal if you get failures.

### Common issues

#### CI Errors

If you get an error in the CI step `Run cmake --build build` that includes near the end of the error:
```
ERROR: Couldn't generate bindings for /home/runner/work/accesskit/accesskit/bindings/c.
```
Somewhere in the big block of text in the error message it also says:
```
unknown feature `proc_macro_span_shrink`
```

This is likely not an issue with your PR and just means `proc-macro2` needs to be updated in our Cargo.lock file.
You probably want to make this change in a separate PR as `proc-macro2` can update often and your PR could become out of date if it takes a while to be reviewed.
paul-hansen marked this conversation as resolved.
Show resolved Hide resolved