[repo] Contribution guide#2859
Conversation
|
Repository collaborators can run the JMH benchmark suite against this PR by commenting: Optional regression threshold override (Δ% on Time or Alloc/op; defaults to 10%): Only one benchmark run per PR is active at a time — issuing a new |
| - what tests will prove the behavior | ||
| - whether documentation, `CHANGELOG.md`, or `docs/features.md` needs to change | ||
|
|
||
| It is fine to use AI tools to draft an implementation proposal, explore design options, or prepare a first implementation. AI-generated code is welcome when the specification is detailed, the resulting code follows project patterns, and the tests clearly validate the expected behavior. Special attention should be paid to tests because they are the main guardrails for code changes. |
There was a problem hiding this comment.
AI-generated code is welcome when
should we add AI_POLICY.md and link it here?
|
|
||
| Contributions are always welcome: issues, pull requests, questions, ideas, tests, documentation, and examples all help the project. | ||
|
|
||
| ClickHouse Java is maintained by a team responsible for the stability and quality of released artifacts. Because this repository publishes client libraries and drivers used by applications, every contribution needs enough context and validation for maintainers to review its safety. At minimum, a code change should include a clear description and tests that prove the intended behavior. |
There was a problem hiding this comment.
every contribution needs enough context and validation for maintainers to review its safety
I'd suggest rephrase to something > here's what makes them easy to review, otherwise, it reads too defensively
|
|
||
| Contributions are always welcome: issues, pull requests, questions, ideas, tests, documentation, and examples all help the project. | ||
|
|
||
| ClickHouse Java is maintained by a team responsible for the stability and quality of released artifacts. Because this repository publishes client libraries and drivers used by applications, every contribution needs enough context and validation for maintainers to review its safety. At minimum, a code change should include a clear description and tests that prove the intended behavior. |
There was a problem hiding this comment.
Because this repository publishes client libraries and drivers used by applications, every contribution needs enough context and validation for maintainers to review its safety.
I'd even strengthen it
The libraries are foundational infrastructure for the software built on top of them, thus a regression or breaking change in a client propagates to applications that depends on it, often with limited ability for users to work around the issue
|
|
||
| ### Big Contributions | ||
|
|
||
| Large changes are easier to review and merge when they are split into a few smaller pull requests. From our experience, it is often better to prepare the codebase first with focused cleanup, refactoring, or test-only PRs, and then add the feature or behavior change in a later PR. |
There was a problem hiding this comment.
The principle is right but the guidance doesn't have an easy to follow definition. Let's give contributors a concrete threshold. For example (numbers are taken from this study https://www.propelcode.ai/blog/pr-size-impact-code-review-quality-data-study):
As a rough guideline, if a pull request is over 300–400 lines of code (excluding docs and tests), consider whether it can be split. Smaller PRs are reviewed and merged faster, and the savings in review time usually outweigh the overhead of splitting
| - a link to the related issue, when one exists | ||
| - an explanation of the user-visible behavior and compatibility impact | ||
| - tests for the changed behavior, including integration tests when the change interacts with ClickHouse server behavior | ||
| - the local test commands that were run |
There was a problem hiding this comment.
not sure we need that level of noize in every PR. I'd just set expectations: you run test before opening a PR
|
|
||
| Changes to CI workflows are currently restricted. Please discuss CI workflow changes with maintainers before opening a pull request. | ||
|
|
||
| ## Pull Request Checklist |
There was a problem hiding this comment.
I'd add a couple of things to set clearer expectations on both sides:
-
CI must be runnable on the PR. For external contributors, the initial CI run requires approval from a maintainer (as of today, probably should be revisited). Let’s explicitly mention this and commit to triggering it promptly to avoid contributors being stuck waiting. For internal PRs, the CI suite should actually run against the proposed change before the review process begins. If tests are scoped down, skipped, or disabled, the PR should clearly indicate this.
-
Reviews occur when CI is green. Clearly state that the team will review the code once CI passes. Additionally, contributors should either fix PRs with failing CI or move them to the draft. This approach is fair in both directions: contributors have a clear understanding of what means
ready for review, and reviewers avoid spending cycles on changes that are not ready
| @@ -1,125 +1,194 @@ | |||
| ## Getting started | |||
| ClickHouse-JDBC client is an open-source project, and we welcome any contributions from the community. Please share your ideas, contribute to the codebase, and help us maintain up-to-date documentation. | |||
| # Contributing to ClickHouse Java | |||
There was a problem hiding this comment.
Most of the guidance provided in the document (Testing, Contribution Process, significant contributions, and the PR checklist) is also relevant to AI coding agents. Considering the significant portion of our contributions made by agents, we should either mirror these rules into AGENTS.md or extract them into a shared file that both documents reference.
This can be done in a separate PR.
|
|
||
| Some good ideas need time to design and implement. We are open to discussion and welcome collaboration in issues, even when the final implementation is not ready yet. | ||
|
|
||
| ### Big Contributions |
There was a problem hiding this comment.
One logical change per PR: a feature, a bug fix, a refactor, or a doc update. Mixed-purpose PRs complicate reviews and increase the likelihood of unintended regressions.
|
|
||
| Large changes are easier to review and merge when they are split into a few smaller pull requests. From our experience, it is often better to prepare the codebase first with focused cleanup, refactoring, or test-only PRs, and then add the feature or behavior change in a later PR. | ||
|
|
||
| Avoid packing unrelated cleanup, infrastructure changes, test rewrites, and the feature itself into one large pull request. Smaller PRs help maintainers verify intent, reduce review time, and lower the risk of regressions. |
There was a problem hiding this comment.
For a new feature, implement the smallest change that solves the problem. Polish, additional configuration, optimization, and extensions can be addressed in the follow-up PRs
| - a link to the related issue, when one exists | ||
| - an explanation of the user-visible behavior and compatibility impact | ||
| - tests for the changed behavior, including integration tests when the change interacts with ClickHouse server behavior | ||
| - the local test commands that were run |
There was a problem hiding this comment.
- run the project's code review skill against your diff before requesting human review
this and run tests locally can be moved to before you open a PR
|
|
||
| Every pull request should include: | ||
|
|
||
| - a clear description of what changed and why |
There was a problem hiding this comment.
| - a clear description of what changed and why | |
| - a clear description of what changed, why and why now |
3 participants
Summary
Reworked contribution guide to tell more about process and expectations.
We should value every ones time and reduce review cycles so we explain how to prepare PR for best result.