-
Notifications
You must be signed in to change notification settings - Fork 888
CASSJAVA-30: Update CONTRIBUTING.md #2041
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
base: 4.x
Are you sure you want to change the base?
Conversation
| but in no way discouraged: it's generally a good idea to have a class-level comment that explains | ||
| where the component fits in the architecture, and anything else that you feel is important. | ||
| - **API packages:** must be documented. | ||
| - **Internal packages:** optional but encouraged—add a short class-level comment explaining the component’s role. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encouraged - add
| Testing against an external process, using either of the tools: | ||
| - [Simulacron](https://github.com/datastax/simulacron): simulates Cassandra nodes on loopback. You “prime” it with expected query results. | ||
| *Example*: `NodeTargetingIT`. | ||
| - [CCM](https://github.com/pcmanus/ccm): launches actual Cassandra nodes locally. The `ccm` executable must be in the path. Pass variables like `-Dccm.version=5.0.0 -Dccm.dse=false` to specify the Cassandra version or distribution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be -ccm.distribution=dse, not -Dccm.dse=false.
| 1. Nullability: Please use the [Spotbugs annotations](https://spotbugs.github.io) to document nullability of new class or interface using the `@Nullable` and `@NonNull` annotations from the package `edu.umd.cs.findbugs.annotations`. | ||
| 2. Concurrency annotations: Please use the [JCIP annotations](http://jcip.net/annotations/doc/index.html) to document thread-safety. | ||
| 3. Static imports: Please only use static imports for AssertJ, Mockito, and Awaitility. | ||
| 4. `toString()`: only for debug in development. Use `toCqlLiteral()` is applicable for logs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorten format does not tell the meaning of previous long description. Not all classes would have toCqlLiteral().
| #### Integration Tests | ||
| 1. Install Cassandra Cluster Manager (CCM) following its [README](https://github.com/apache/cassandra-ccm). | ||
| 2. On macOS, enable loopback aliases: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe explicitly mention On MacOS only, ...
|
I feel like the current state of this PR loses a lot of good suggestions and guidance about how new code should be structured. I'm wondering if we can't make a smaller set of edits to update what needs to be changed while still keeping most of the guidance in the current doc. |
|
I think realistically, no community contributor is gonna read 500+ lines before they submit a single PR. It makes sense for the onboarding of a full-time job, but it does not make sense for a community contributor, e.g., someone who maintains a library that uses the Java driver as a dependency and only plans to create one pull request. When they open such a long file, even if the file is nice and detailed, they are just gonna skip it. If you agree ^ this is true, then we gotta make it shorter. My 2 cents is that the following has lower priority and can be potentially removed:
It's rare that a community contributor submits a PR with this functional stream API. And when it happens, we can fix it in the PR review. Such things should be of low priority. I think those points ^ are all good to have, but are less important than a short CONTRIBUTING.md so that people actually read. But if you both think the longer version is better, I will rewrite it. Pls let me know what you think :) |
No description provided.