From 13cd1db70343a865b7d182f20f11b1d24d69e798 Mon Sep 17 00:00:00 2001 From: IntegratedQuantum Date: Wed, 25 Sep 2024 20:45:47 +0200 Subject: [PATCH] Add contributing guidelines. Probably still incomplete, but should be good start. --- CONTRIBUTING.md | 87 +++++++++++++++++++++++++++++++++++++++++++++++++ README.md | 3 +- 2 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 000000000..951ffc312 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,87 @@ +# About this document + +This document contains a set of guidelines to help you contribute to Cubyz in a smooth and efficient manner. + +Making a pull requests that go through multiple rounds of reviews before getting merged is annoying for everyone involved. + +The sections are roughly sorted by the time you'll encounter them, starting before selecting what to work on and ending after you made your pull request. + +I'd recommend to check out the [Discord Server](https://discord.gg/XtqCRRG) if you have any further questions. + +# Learn Zig + +Kind of obvious, but still important: https://ziglang.org/learn/ + +If you are new to Zig it can also be very helpful to ask questions. For example, if something feels annoying to write, then you might be missing knowledge about better approaches. Asking a question is usually faster than writing 100 lines of cumbersome code (and it saves a review cycle). + +# Disable Zig's automatic formatter + +This is a common beginners mistake that can lead to a ton of work undoing the magic of the auto formatter. +In VSCode you can disable this in the Zig extension settings. + +# Select something to work on + +The best way to start is obviously the issues tab. The issues are organized with labels (most importantly the Contributor friendly label) and milestones, so it should be easy to find something. + +But of course the ever-growing list of issues is not complete and other changes are welcome as well, as long as they are not going in a completely different direction. And also make sure to explain what you are trying to do in the pull request description. + +You might also find some ideas on [Discord](https://discord.gg/XtqCRRG). + +# Start ₛₘₐₗₗ + +Especially as a first time contributor it is likely that your code is not meeting the standards of Cubyz. This is totally normal, but obviously it means that your changes will go through more review cycles. + +To have more success it would help to split things up into smaller PRs, maybe start by doing some preliminary changes leading up to feature, for example you could start by just introducing some new utility functions you will need for the actual feature. And of course it can be helpful to ask first if you are even going in the right direction. + +This saves time on your end spent reworking your large pull request 10 times. And reviewing your large pull request 10 times is also not fun. + +# Write correct and readable code + +## Explicitly handle all errors (except from those that can't happen) + +Error handling usually means logging the error and continuing with a sensible default. For example if you can't read a file, then the best solution would be to write a message to `std.log.err` that states the file path and the error string. It is also fine to bubble up the error with `try` a few levels before handling it. + +Not all errors can happen. Specifically in Cubyz the `error.OutOfMemory` cannot happen with the standard allocators (`main.globalAllocator` and `main.stackAllocator`). In this case `catch unreachable` is the right way to handle the error. + +## Choose the right allocator for the job + +Cubyz has two main allocators. +- The `main.stackAllocator` is a thread-local allocator that is optimized for local allocations. Use for anything that you free at the end of the scope. An example use-case would be a local `List`. +- The `main.globalAllocator` is intended to be used for general purpose use cases that don't need to or can't be particularly optimized. + +Sometimes it might also make sense to use an arena allocator `utils.NeverFailingArenaAllocator`, or a `MemoryPool`. But generally these should only be used where it makes sense. + +Also it is important to note that Cubyz uses a different allocator interface `utils.NeverFailingAllocator` which cannot return `error.OutOfMemory`. Along with it come some data structures like `main.List` and more in `utils` which should be preferred over the standard library data structures because they simplify the code. + +## Free all resources + +Everything you allocate, needs to be freed. +Everything you init needs to be deinited. +Everything you lock needs to be unlocked. +This needs to be true even for error paths such as introduced by `try`. +Usually you should be fine by using `defer` and `errdefer`. There is also leak-checking in debug mode, so make sure you test your feature in debug mode to check for leaks. + +## Try to follow the style of the existing code + +There is no style guide yet, so this has to be good enough for now. A few important rules (tabs and line endings) are ensured using the CI. + +# Don't put multiple changes into one pull request + +It may seem tempting to bundle up somewhat related features into one pull request. But this often causes unnecessary delays. + +Let's say you have 3 features and made a small mistake in one of them. + +If you bundle up all 3 features, then because of your small mistake, a review cycle is needed. +But by the time you fixed the mistake, maybe someone else worked on a file you touched and there are merge conflicts. The more changes you bundle into one PR the more likely it is that this happens. This is especially bad if one of your actions is refactoring a bunch of code. +And even if it doesn't happen, now the same code has to be reviewed again in it's entirety. + +If instead you make 3 separate PRs, the first two can be merged on the same day, while the last one needs to be edited. The chances of a merge conflict are small, since 2/3 changes are already merged. And code review is also easier, since only the broken code has to be reviewed more than once. + +# Check the changes after creating your pull request + +With a quick check you can ensure that you didn't add any unintended changes, like for example unintended formatting changes from some kind of automatic formatter. Some editors for example introduce tabs on random lines or change line endings. + +With a more thorough review of your changes you can sometimes catch small mistakes, leftover TODOs or random debug code. + +And of course make sure to check the CI results, you should also get an e-mail notification if the CI fails. + diff --git a/README.md b/README.md index 5537bf154..d68425c60 100644 --- a/README.md +++ b/README.md @@ -50,8 +50,7 @@ sudo apt install libgl-dev libasound2-dev libx11-dev libxcursor-dev libxrandr-de # Contributing ### Code -Try to follow the style of the existing code. The `format_check.py` file can be used to find violations of indentation and line endings. `// TODO: Add a full style guide`
-If you have any more questions, you can ask them over on [Discord](https://discord.gg/XtqCRRG). +Check out the [Contributing Guidelines](https://github.com/PixelGuys/Cubyz/blob/master/CONTRIBUTING.md) ### Textures If you want to add new textures, make sure they fit the style of the game. It's recommended that you have baseline skills in pixel art before attempting to make textures. A great collection of tutorials can be found [here](https://lospec.com/pixel-art-tutorials)