| Age | Commit message (Collapse) | Author |
|---|
|
Fix typos found by typos-cli(https://github.com/crate-ci/typos). Some
affected tests are adjusted.
There are a bunch of other typos are ignored, including
* CHANGELOG.md
* NOTICE
* internal/.../migrations/20201208163237_cleanup_notifications_payload.go
* other intended typos or false positives
Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
|
|
Update our STYLE.md to reflect modern best practices around
context-aware logging. Most importantly, we don't use the ctxlogrus
package anymore and instead use context-aware logging functions like
`DebugContext()` and related functions.
|
|
Introduce our own wrapper type for the `logrus.Fields` type. This allows
us to get rid of the logrus import in many places and thus is another
step towards getting rid of the `logrus` dependency altogether in our
code base.
|
|
The `ctxlogrus` package is about to go away. Let's encapsulate it so
that the transition will become easier for us at a later point.
|
|
Introduce a wrapper for the logrus logging infrastructure. This is a
first step towards abstracting away the actual implementation of our
logs behind a type that we can iterate on and will thus pave the way
towards adoption of the `slog` package.
Note that this is only one step of this transition. The interface does
not yet roundtrip to itself, e.g. calling `log.Logger.WithError()` will
result in a `logrus.Entry`, not in a `log.Logger`. This will be handled
at a later point.
|
|
The prior commit fixes inelligible usages of Unavailable status codes.
To prevent this situation from happening in the future, this commit
implements a linter that it warns occurrences where Unavailable code is
used. Engineers can ignore it by adding a comment, but at least this
practice catches their eyes.
|
|
The `structerr.New()` constructor returns errors with an `Internal`
code. This makes for a somewhat weird interface though: `New()` shall be
used in cases where the actual error condition is not quite clear, but
by already setting an `Internal` error code we'll ultimately propagate
that code even if an ancestor in the callchain would know better.
Adjust the function to instead return `Unknown` errors. Together with
the change to always override `Unknown` errors with more specific ones,
this makes for a new calling convention of how to construct errors:
always use `New()`, unless you know the actual root cause. Like this,
the first caller that knows more about the root cause will override the
`Unknown` error.
Document this calling convention.
|
|
|
|
|
|
We have recently introduced a new `structerr` package that is intended
to replace the `helper.Err*()` helper functions. Rewrite the section
about error creation in our style guide to match the new recommended way
of creating errors.
|
|
The new functions ErrDataLossf(), ErrUnknownf() and
ErrUnimplementedf() will be in use soonish. They are
doing the same as other already existing code-based
errors.
The documentation updated with a short description
why helper wrappers should be used for error creation.
Part of https://gitlab.com/gitlab-org/gitaly/-/issues/4471
|
|
Also expand reach of Markdown linting to more files
|
|
Document the order of arguments that we use in this project to be
`testing.TB` first, `context.Context` second to make this discoverable.
While at it also document our use of `t.Helper()`.
|
|
With the upcoming transition to start supporting the SHA256 object hash
in our repositories we will have to amend our tests to support both SHA1
and SHA256 object hashes. Due to our use of static seed repositories in
many of our tests though this proving to be quite a burden as we have an
abundance of hardcoded SHA1 hashes.
Discourage the use of seed repositories in favor of generating the data
at runtime instead with our test helpers. This gives us agility when the
object format of a repository changes as we don't depend on hardcoded
information anymore, but in many cases it also speeds up test execution
as we don't have to clone thousands of repositories on every test run
anymore. Last but not least this also helps readers understand tests
better given that they don't have to peek into the seed repositories to
get an understanding of how the test data is arranged, but instead can
learn about it by just reading through the now-explicit test data setup.
|
|
It makes more sense to mention the commit guidelines in CONTRIBUTING.md,
so move them over from STYLE.md. This will bring the commit and
changelog guidelines together as they are tightly related.
|
|
Setup of a test package's main function is quite repetitive: we call a
separate `testMain()` function such that we can use `defer` calls, this
function calls `MustHaveNoChildProcess()` and `Configure()`, and then we
return the result of `m.Run()`. This is almost always exactly the same,
with some exceptions where we need to have additional setup code.
Unify this code via a single `testhelper.Run()` function which does all
of this. It allows us greater flexibility in the setup code such that we
can easily perform additional validation after the tests without having
to modify all callsites.
Note that this change removes calls to the goleak package in some
places. These will resurface in a later commit in this patch series,
where we instead move this call into `testhelper.Run()` such that it is
executed by default.
|
|
When a test name suffix consists of multiple words camelCasing should be
used to indicate separation of words.
|
|
Use %w to wrap errors
See merge request gitlab-org/gitaly!2877
|
|
Code style document updated to notice on the error wrapping and useful links for reference.
|
|
Since v2.25.0 of git you've been able to use this way of doing it. See
git-vcs/git@1f0fc1db85 (pretty: implement 'reference' format,
2019-11-19).
So let's use that example, and while I'm at it change the SHA-1 in the
example here to the one that landed in the repo, not some pre-rebase
version that was only in @pks-t's local repo.
|
|
While our style document currently contains mostly advice about code
hygiene, commit hygiene is also important for a project to ensure it
ends up with understandable commits. It ensures that it is easily
possible to dig up the reasoning behind changes in the future, making
the project easier to maintain in the long run.
This commit thus documents various best practices around commit hygiene.
|
|
style: Document location of `TestMain()` function
See merge request gitlab-org/gitaly!2799
|
|
While many tests use a `testhelper_test.go` file to implement
`TestMain()`, this is not common to all. This makes test setup harder to
find than necessary. This commit thus documents the recommended name of
"testhelper_test.go" for such code.
|
|
Using `os.Exit()` will cause the calling process to exit immediately
without executing any deferred functions. In the context of our tests,
this means that any created ondisk state may not get cleaned up
correctly. This commit thus documents those pitfalls in "STYLE.md".
|
|
|
|
Go vet, editors, and other tooling will emit warnings when a struct
doens't have the fields names, as it falls back on ordering. That could
introduce bugs when new fields are added with the same type at the
beginning of a struct.
To reduce the number of warnings in my editor, I ran a `sed` script to
update the code.
|
|
Our current policy with regards to pointer vs. value receivers is not
documented anywhere. Given that we have a policy saying that if a single
method requires a pointer receiver, all methods should be pointer
receivers, there's some non-obvious bits to our coding style. So let's
write this part down so we can point to it from now on.
|
|
There's currently a lot of different practices sprawling throughout the
codebase when it comes to logging, so chances are high you will not use
up-to-date best practices when writing new logging code. So let's nail
down our logging style so we have a document we can consult and point
to.
|
|
Developer-facing code should use the ASCII character set as far as
possible to ensure improve accessibility across platforms. Many
platforms lack a lot of UTF8 code glyps, leading to code that is hard to
read on such platforms if they are used.
Add a note to our style guide that mandates this rule.
|
|
Add a note on ordering of functions and types
See merge request gitlab-org/gitaly!1949
|
|
Types should be declared before their first. Let's document this in our
style guide.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
This reverts merge request !653
|
|
|
|
|
|
|
|
|
|
|
|
|
