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: update CONTRIBUTING.md guide #81

Merged
merged 3 commits into from
Dec 4, 2024
Merged

Conversation

ttypic
Copy link
Collaborator

@ttypic ttypic commented Dec 4, 2024

Added section about:

  • building
  • testing
  • code style
  • using local build as a dependency

Summary by CodeRabbit

  • Documentation

    • Expanded the CONTRIBUTING.md file with new sections on building the project, code standards, and running tests.
    • Added detailed instructions for using the Gradle wrapper, running Detekt for code style checks, and executing JUnit tests.
    • Enhanced clarity on using chat-android locally and updated the "Release Process" section for better understanding.
  • Chores

    • Adjusted dependency resolution settings by removing mavenLocal() from the build.gradle.kts file.
    • Updated Detekt report configurations and ensured the check task runs the detekt task during checks.

Copy link

coderabbitai bot commented Dec 4, 2024

Walkthrough

The changes in this pull request involve significant enhancements to the CONTRIBUTING.md file, adding new sections on building the project, code standards, and running tests. Additionally, the build.gradle.kts file has been modified to remove the mavenLocal() repository and update the Detekt configuration for reporting. These updates aim to clarify and improve the contributing guidelines and build process for developers.

Changes

File Change Summary
CONTRIBUTING.md Expanded to include sections on building the project (using Gradle wrapper), code standards (introducing Detekt), and running tests (JUnit). Detailed steps added for local usage of chat-android. Minor adjustments made to the "Release Process" section.
build.gradle.kts Removed mavenLocal() from allprojects, updated Detekt reports configuration to require Markdown reports, registered check task to run detekt, and ensured proper Maven publishing plugin configuration for subprojects.

Possibly related PRs

Suggested reviewers

  • sacOO7

🐇 In the garden, we hop and play,
With guidelines clear, we find our way.
Building and testing, all in line,
Contributing's fun, oh how divine!
So grab your code and join the cheer,
Together we'll code, the time is near! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@ttypic ttypic requested a review from sacOO7 December 4, 2024 12:52
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (7)
CONTRIBUTING.md (7)

14-15: Add a comma for better readability

Consider adding a comma before 'so' to improve the sentence structure.

-A gradle wrapper is included so these tasks can run without any prior installation of gradle. The Linux/OSX form of the commands, given
+A gradle wrapper is included, so these tasks can run without any prior installation of gradle. The Linux/OSX form of the commands, given
🧰 Tools
🪛 LanguageTool

[uncategorized] ~14-~14: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...# Building A gradle wrapper is included so these tasks can run without any prior i...

(COMMA_COMPOUND_SENTENCE_2)


29-31: Consider documenting available modules

The build command specifically targets the 'chat-android' module. Consider listing all available modules and their purposes for better clarity.


41-47: Enhance Detekt documentation

Consider adding:

  1. Information about interpreting Detekt reports
  2. Common issues and how to fix them
  3. Location of Detekt configuration file

49-58: Expand test documentation

Consider enhancing this section with:

  1. Test organization/structure
  2. Different types of tests available
  3. Location and interpretation of test reports
  4. Instructions for writing new tests

61-62: Simplify the introduction

Consider rephrasing for clarity and conciseness.

-If you wish to make changes to the source code and test them immediately in another project, follow these steps:
+To test local changes in another project, follow these steps:
🧰 Tools
🪛 LanguageTool

[style] ~61-~61: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cally in other projects If you wish to make changes to the source code and test them immediate...

(MAKE_CHANGES)


67-69: Use placeholder version number

Consider using a more generic placeholder to prevent confusion.

-VERSION_NAME=0.1.0-local.1
+VERSION_NAME=X.Y.Z-local.1  # Replace X.Y.Z with the current version

133-135: Use placeholder version in example

Consider using placeholders in the example command to prevent it from becoming outdated.

-github_changelog_generator -u ably -p ably-chat-kotlin --since-tag v1.2.3 --output delta.md --token $GITHUB_TOKEN_WITH_REPO_ACCESS
+github_changelog_generator -u ably -p ably-chat-kotlin --since-tag vX.Y.Z --output delta.md --token $GITHUB_TOKEN_WITH_REPO_ACCESS  # Replace X.Y.Z with the previous version
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b84b113 and 46f4d18.

📒 Files selected for processing (2)
  • CONTRIBUTING.md (3 hunks)
  • build.gradle.kts (0 hunks)
💤 Files with no reviewable changes (1)
  • build.gradle.kts
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md

[uncategorized] ~14-~14: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...# Building A gradle wrapper is included so these tasks can run without any prior i...

(COMMA_COMPOUND_SENTENCE_2)


[style] ~61-~61: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cally in other projects If you wish to make changes to the source code and test them immediate...

(MAKE_CHANGES)

Copy link

github-actions bot commented Dec 4, 2024

Code Coverage

Total Project Coverage 83.33%

Added section about:

- building
- testing
- code style
- using local build as a dependency
@ttypic ttypic force-pushed the update-contributing-guide branch from 46f4d18 to 0323879 Compare December 4, 2024 16:20
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (7)
CONTRIBUTING.md (7)

14-15: Minor grammatical improvement needed

Consider adding a comma for better readability:

-A gradle wrapper is included so these tasks can run without any prior installation of gradle. The Linux/OSX form of the commands, given
+A gradle wrapper is included, so these tasks can run without any prior installation of gradle. The Linux/OSX form of the commands, given
🧰 Tools
🪛 LanguageTool

[uncategorized] ~14-~14: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...# Building A gradle wrapper is included so these tasks can run without any prior i...

(COMMA_COMPOUND_SENTENCE_2)


27-31: Add output location information

Consider adding information about where the built AAR package will be located after the build command.

To build the library's AAR package, execute:

```bash
./gradlew chat-android:assemble

+The built AAR package will be available in the chat-android/build/outputs/aar/ directory.


---

`41-47`: **Enhance Detekt documentation**

Consider adding information about:
1. How to interpret and handle Detekt findings
2. Whether Detekt checks are enforced in CI/CD

```diff
#### Running Detekt Locally

You can run Detekt locally to verify your code against the configured rules. Use the following command:

```bash
./gradlew detekt

+The command will generate a report highlighting any issues found. The report can be found at build/reports/detekt/.
+
+> [!NOTE]
+> Detekt checks are also run automatically in our CI/CD pipeline for all pull requests.


---

`54-62`: **Expand test documentation**

Consider adding more detailed information about:
1. Different types of tests (unit, integration, etc.)
2. Test report locations
3. How to debug test failures

```diff
### Running Tests

Our tests are written using JUnit and can be executed with the following command:

```bash
./gradlew test

This will run all unit tests in the project. Ensure all tests pass before submitting a pull request.
+
+#### Test Reports
+Test reports can be found in the build/reports/tests/ directory after running the tests.
+
+#### Debugging Test Failures
+For detailed test output, you can run:
+bash +./gradlew test --info +


---

`66-67`: **Consider rephrasing for conciseness**

The current wording could be more direct:

```diff
-If you wish to make changes to the source code and test them immediately in another project, follow these steps:
+To test local changes in another project, follow these steps:
🧰 Tools
🪛 LanguageTool

[style] ~66-~66: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cally in other projects If you wish to make changes to the source code and test them immediate...

(MAKE_CHANGES)


101-104: Add cleanup instructions

Consider adding instructions for cleaning up local development setup:

> [!NOTE]
> - Ensure the version number (`0.1.0-local.1`) matches the one you set in gradle.properties.
> - The `mavenLocal()` repository should typically be used only during development to avoid conflicts with published versions in remote
    repositories.
+> - After testing, remember to remove the mavenLocal() repository and revert to the published version to avoid unintended usage of local builds.

138-145: Fix indentation in changelog section

The indentation of the list items could be improved for better readability:

-    - The command you will need to run will look something like this:
-      `github_changelog_generator -u ably -p ably-chat-kotlin --since-tag v1.2.3 --output delta.md --token $GITHUB_TOKEN_WITH_REPO_ACCESS`.
-      Generate token [here](https://github.com/settings/tokens/new?description=GitHub%20Changelog%20Generator%20token).
-    - Using the command above, `--output delta.md` writes changes made after `--since-tag` to a new file.
-    - The contents of that new file (`delta.md`) then need to be manually inserted at the top of the `CHANGELOG.md`, changing the "
-      Unreleased"
-      heading and linking with the current version numbers.
-    - Also ensure that the "Full Changelog" link points to the new version tag instead of the `HEAD`.
+- The command you will need to run will look something like this:
+  `github_changelog_generator -u ably -p ably-chat-kotlin --since-tag v1.2.3 --output delta.md --token $GITHUB_TOKEN_WITH_REPO_ACCESS`.
+  Generate token [here](https://github.com/settings/tokens/new?description=GitHub%20Changelog%20Generator%20token).
+- Using the command above, `--output delta.md` writes changes made after `--since-tag` to a new file.
+- The contents of that new file (`delta.md`) then need to be manually inserted at the top of the `CHANGELOG.md`, changing the "Unreleased"
+  heading and linking with the current version numbers.
+- Also ensure that the "Full Changelog" link points to the new version tag instead of the `HEAD`.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 46f4d18 and 0323879.

📒 Files selected for processing (2)
  • CONTRIBUTING.md (3 hunks)
  • build.gradle.kts (0 hunks)
💤 Files with no reviewable changes (1)
  • build.gradle.kts
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md

[uncategorized] ~14-~14: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...# Building A gradle wrapper is included so these tasks can run without any prior i...

(COMMA_COMPOUND_SENTENCE_2)


[style] ~66-~66: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cally in other projects If you wish to make changes to the source code and test them immediate...

(MAKE_CHANGES)

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
Co-authored-by: sachin shinde <sachinshinde7676@gmail.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (7)
CONTRIBUTING.md (7)

27-31: Add information about the output location

The command explanation should include where to find the built artifact.

Add the output location information:

 ```bash
 ./gradlew chat-android:assemble

+The built AAR package will be available in the chat-android/build/outputs/aar/ directory.


---

`51-52`: **Use relative link for configuration file**

Using an absolute GitHub URL can break when the repository is forked.

```diff
-Detekt's rules can be updated by modifying the [detekt.yml](https://github.com/ably/ably-chat-kotlin/blob/main/detekt.yml) configuration
+Detekt's rules can be updated by modifying the [detekt.yml](./detekt.yml) configuration

43-47: Add information about viewing Detekt reports

The section should include where to find the Detekt reports after running the check.

 You can run Detekt locally to verify your code against the configured rules. Use the following command:

 ```bash
 ./gradlew detekt

+After running Detekt, you can find the HTML report at build/reports/detekt/detekt.html.


---

`58-62`: **Add information about test reports location**

Include where developers can find the test reports for debugging failures.

```diff
 ```bash
 ./gradlew test

+Test reports can be found in the build/reports/tests/test/index.html file.


---

`66-67`: **Improve clarity of the introduction**

The current wording could be more concise.

```diff
-If you wish to make changes to the source code and test them immediately in another project, follow these steps:
+To test source code changes immediately in another project, follow these steps:
🧰 Tools
🪛 LanguageTool

[style] ~66-~66: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cally in other projects If you wish to make changes to the source code and test them immediate...

(MAKE_CHANGES)


73-74: Use placeholder version numbers

Using specific version numbers in examples can become outdated. Consider using placeholder versions.

-VERSION_NAME=0.1.0-local.1
+VERSION_NAME=X.Y.Z-local.1
-    implementation("com.ably.chat:chat-android:0.1.0-local.1")
+    implementation("com.ably.chat:chat-android:X.Y.Z-local.1")

Also applies to: 98-99


142-144: Fix line wrapping in changelog instructions

The word "Unreleased" is split across lines, which affects readability.

-    heading and linking with the current version numbers.
-    - Also ensure that the "Full Changelog" link points to the new version tag instead of the `HEAD`.
+    heading and linking with the current version numbers.
+    - Also ensure that the "Full Changelog" link points to the new version tag instead of the `HEAD`.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 0323879 and df2752f.

📒 Files selected for processing (1)
  • CONTRIBUTING.md (3 hunks)
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md

[style] ~66-~66: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cally in other projects If you wish to make changes to the source code and test them immediate...

(MAKE_CHANGES)

@ttypic ttypic requested a review from sacOO7 December 4, 2024 17:05
Copy link
Contributor

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

LGTM, I have picked up some suggestions from coderabbitai nitpick comments : P
You can check those comments again if needed.
Thanks for adding CONTRIBUTING.md

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
CONTRIBUTING.md (5)

14-14: Fix grammatical issue in the sentence

The sentence ends awkwardly with "given". Consider rewording for better clarity.

-A gradle wrapper is included, so these tasks can run without any prior installation of gradle. The Linux/OSX form of the commands, given
+A gradle wrapper is included, so these tasks can run without any prior installation of gradle. The Linux/OSX form of the commands is:

41-41: Add information about handling Detekt findings

Consider adding guidance on how to interpret and address Detekt findings, including:

  • Where to find the Detekt report
  • How to handle false positives
  • Common issues and their solutions

64-64: Enhance test documentation with additional details

Consider adding information about:

  • Different types of tests available (unit, integration, etc.)
  • Location of test reports
  • How to debug failed tests
  • Test coverage requirements

68-68: Improve sentence structure for clarity

The current wording is a bit verbose. Consider a more direct approach.

-If you wish to make changes to the source code and test them immediately in other projects, follow these steps:
+To test source code changes immediately in other projects, follow these steps:
🧰 Tools
🪛 LanguageTool

[style] ~68-~68: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cally in other projects If you wish to make changes to the source code and test them immediate...

(MAKE_CHANGES)


140-147: Enhance changelog generation documentation

Consider adding:

  1. Example of the expected changelog format
  2. Security note about handling the GitHub token
  3. Explanation of when to use different changelog generator options
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between df2752f and ec0dfce.

📒 Files selected for processing (1)
  • CONTRIBUTING.md (3 hunks)
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md

[style] ~68-~68: Consider shortening or rephrasing this to strengthen your wording.
Context: ...cally in other projects If you wish to make changes to the source code and test them immediate...

(MAKE_CHANGES)

@ttypic ttypic merged commit cb58c0b into main Dec 4, 2024
3 checks passed
@ttypic ttypic deleted the update-contributing-guide branch December 4, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants