Make overwritable experimental behind a cargo feature
#160
Conversation
|
Warning Rate limit exceeded@Veetaha has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the addition of a new feature called Changes
Possibly related PRs
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
website/infra/README.md (2)
13-23: Enhance security guidance for sensitive values.The terraform.tfvars example includes sensitive values but lacks guidance on secure value generation and management.
Consider adding:
- Guidelines for generating secure passwords and tokens
- Instructions to use environment variables or a secrets manager
- Warning about not committing terraform.tfvars to version control
Example addition:
Important security notes: - Generate a strong `umami_app_secret` (at least 32 characters) - Use a password manager to generate secure `pg_password` - Never commit terraform.tfvars to version control - Consider using environment variables or a secrets manager in production
7-35: Documentation could be more comprehensive.The deployment section would benefit from additional information to ensure successful deployment and maintenance.
Consider adding:
- Required/supported umami version
- Description of what each placeholder means and their format requirements
- Post-deployment verification steps
- Backup strategy for the Postgres volume
- Monitoring recommendations
- Troubleshooting guide
Example addition:
## Post-deployment 1. Verify deployment: ```bash terraform output # Visit the displayed IP address to confirm umami is running
- Backup strategy:
- The Postgres data is stored on a separate volume at [location]
- Recommended backup procedure: [steps]
</blockquote></details> <details> <summary>bon/Cargo.toml (1)</summary><blockquote> `105-120`: **LGTM! Consider adding a version note.** The feature declaration and documentation are well-structured and comprehensive. The experimental status, compatibility guarantees, and purpose are clearly communicated. The community engagement approach through GitHub reactions is a good practice. Consider adding a version note to help users track when this feature was introduced: ```diff # 🔬 Experimental! There may be breaking changes to this feature between *minor* releases, # however, compatibility within patch releases is guaranteed though. +# +# This feature was introduced in bon 2.3.0. # # This feature enables the #[builder(overwritable)] attribute that can be used towebsite/changelog.md (2)
Line range hint
35-35: Update changelog entry foroverwritablefeatureThe changelog entry for
#[builder(overwritable)]should mention that this feature is experimental and requires enabling theexperimental-overwritablecargo feature. This aligns with the PR objective of making the feature experimental.Apply this diff to update the changelog entry:
-Add `#[builder(overwritable)]` and `#[builder(on(..., overwritable)]` to make it possible to call setters multiple times for the same member ([#145](https://github.com/elastio/bon/pull/145)) +Add `#[builder(overwritable)]` and `#[builder(on(..., overwritable)]` to make it possible to call setters multiple times for the same member (requires `experimental-overwritable` feature) ([#145](https://github.com/elastio/bon/pull/145))
48-48: Enhance documentation for analytics implementationThe addition of anonymous statistics tracking is a significant change that affects user privacy. Consider expanding this changelog entry to:
- Describe what data is being collected
- Explain how users can opt out of tracking
- Link to the privacy policy or data collection documentation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
bon-macros/src/builder/builder_gen/member/config/mod.rsis excluded by!**/*.rsbon-macros/src/builder/builder_gen/top_level_config/on.rsis excluded by!**/*.rs
📒 Files selected for processing (7)
- .github/workflows/ISSUE_TEMPLATE/main.md (1 hunks)
- .github/workflows/ci.yml (1 hunks)
- bon-macros/Cargo.toml (1 hunks)
- bon/Cargo.toml (1 hunks)
- e2e-tests/Cargo.toml (1 hunks)
- website/changelog.md (1 hunks)
- website/infra/README.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ISSUE_TEMPLATE/main.md
🧰 Additional context used
📓 Learnings (1)
e2e-tests/Cargo.toml (1)
Learnt from: Veetaha PR: elastio/bon#145 File: benchmarks/runtime/Cargo.toml:34-34 Timestamp: 2024-10-20T00:49:04.064Z Learning: In the `bon` project, the `bon` crate has been moved into a subfolder. Therefore, in `benchmarks/runtime/Cargo.toml`, the dependency path for `bon` is correctly set to `"../../bon"`. Future code reviews should consider this project structure when verifying dependency paths.
🪛 LanguageTool
website/infra/README.md
[uncategorized] ~3-~3: Possible missing comma found.
Context: ...code for our umami backend used for collecting anonymous statics a...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (4)
e2e-tests/Cargo.toml (2)
21-21: LGTM! Feature flag addition is appropriate.The addition of the
experimental-overwritablefeature aligns with the PR objective to make theoverwritablefunctionality experimental. This is particularly appropriate in a test crate where we want to verify the experimental features.
21-21: Verify the bon crate path.The path looks correct for the current location, but let's verify it matches the project structure.
✅ Verification successful
The script results confirm that
bon/Cargo.tomlexists at the repository root, and the relative path../bonfrome2e-tests/Cargo.tomlis correct since both files are at the root level. Let me generate the final response:Path verification successful
The relative path
../bonine2e-tests/Cargo.tomlcorrectly points to theboncrate, and the experimental feature flag addition is appropriate for the test crate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the bon crate location relative to e2e-tests # Expected: Find Cargo.toml in ../bon relative to e2e-tests # Test 1: Check if bon/Cargo.toml exists at the expected path if [ -f "bon/Cargo.toml" ]; then echo "Found bon/Cargo.toml at repository root" fi # Test 2: List all Cargo.toml files to verify project structure fd Cargo.tomlLength of output: 326
.github/workflows/ci.yml (1)
98-101: LGTM! Comprehensive test coverage for the experimental feature.The new test commands properly validate the
experimental-overwritablefeature across all possible feature combinations:
- Base feature
- With alloc
- With implied-bounds
- With both alloc and implied-bounds
The use of
--no-default-featuresensures proper isolation of the feature flags.Let's verify the feature flag is properly defined:
✅ Verification successful
Feature flag is properly defined and gated behind experimental feature
The verification confirms that:
- The
experimental-overwritablefeature is properly defined inbon-macros/Cargo.tomland propagated to the main crate throughbon/Cargo.toml- The feature is properly gated in code with runtime checks:
if !cfg!(feature = "experimental-overwritable") && self.overwritable.is_present() { bail!( &self.overwritable.span(), "🔬 `overwritable` attribute is experimental and requires \"experimental-overwritable\" cargo feature to be enabled..." ); }- The feature is properly documented with a link to issue Stabilize
#[builder(overwritable)]#149 for feedback- The implementation is consistent across both member-level and type-level configurations
The CI workflow changes correctly test all feature combinations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify experimental-overwritable feature is properly defined # Expected: Feature should be defined in Cargo.toml files # Check bon/Cargo.toml for the feature definition echo "Checking bon/Cargo.toml for experimental-overwritable feature:" rg "experimental-overwritable" bon/Cargo.toml -A 2 # Check if the feature is used in the code echo -e "\nChecking feature usage in code:" rg "#\[cfg\(feature\s*=\s*\"experimental-overwritable\"\)\]" bon/src/Length of output: 403
Script:
#!/bin/bash # Let's do a more comprehensive check # 1. Check both bon and bon-macros Cargo.toml files echo "=== Checking Cargo.toml files ===" echo "bon/Cargo.toml features section:" rg "\[features\]" bon/Cargo.toml -A 10 echo -e "\nbon-macros/Cargo.toml features section:" rg "\[features\]" bon-macros/Cargo.toml -A 10 # 2. Check for any overwritable-related code in both crates echo -e "\n=== Checking overwritable-related code ===" echo "In bon crate:" rg "overwritable" bon/src/ -l echo -e "\nIn bon-macros crate:" rg "overwritable" bon-macros/src/ -l # 3. Check for feature gates in both crates echo -e "\n=== Checking feature gates ===" echo "In bon crate:" rg "#\[cfg.*feature.*overwritable" bon/src/ echo -e "\nIn bon-macros crate:" rg "#\[cfg.*feature.*overwritable" bon-macros/src/Length of output: 1865
Script:
#!/bin/bash # Let's check the actual implementation of overwritable feature in the macro crate # since we found the relevant files but need to see the feature gates echo "=== Checking overwritable implementation in bon-macros ===" echo "1. Checking setters implementation:" cat bon-macros/src/builder/builder_gen/setters/mod.rs echo -e "\n2. Checking member config implementation:" cat bon-macros/src/builder/builder_gen/member/config/mod.rs echo -e "\n3. Checking top level config:" cat bon-macros/src/builder/builder_gen/top_level_config/on.rs # Also check if there are any tests that verify this feature echo -e "\n=== Checking tests ===" rg "experimental-overwritable" bon-macros/src/tests/ -lLength of output: 37075
website/changelog.md (1)
Line range hint
1-24: LGTM! Well-structured changelogThe changelog follows the Keep a Changelog format and properly documents changes with PR references.
| # See the docs on this feature in the `bon`'s crate `Cargo.toml` | ||
| experimental-overwritable = [] | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Documentation reference in comment is misleading
The comment states "See the docs on this feature in the bon's crate Cargo.toml", but the bon crate's Cargo.toml only contains the feature declaration without any documentation. The feature is used in the codebase:
- Declared in
bon-macros/Cargo.toml - Referenced in
bon/Cargo.tomlfor feature forwarding - Used in runtime checks in
bon-macros/src/builder/builder_gen/files - Used in e2e tests
Consider either:
- Removing the misleading documentation reference comment, or
- Adding actual documentation about this experimental feature in
bon/Cargo.toml
🔗 Analysis chain
LGTM! Feature flag implementation looks correct.
The implementation of the experimental-overwritable feature flag follows Rust's best practices:
- Empty feature flag is appropriate for compile-time toggles
- Clear documentation reference helps maintainability
- Not included in default features, which is correct for experimental features
Let's verify the feature documentation reference:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the referenced documentation exists in bon's Cargo.toml
# Expected: Find documentation about the experimental-overwritable feature
rg -A 5 "experimental-overwritable" ../bon/Cargo.toml
Length of output: 111
Script:
#!/bin/bash
# Let's first check if the bon crate exists in the repository
fd -t d "bon$"
# Then let's check all Cargo.toml files for the feature documentation
fd "Cargo.toml" -x rg -A 5 "experimental-overwritable" {}
# Also search for any documentation about this feature in other common places
rg -i "experimental.?overwritable" -g "!target/" -g "*.{md,rs,toml}"
Length of output: 1330
There was a problem hiding this comment.
The file bon/Cargo.toml contains the feature declaration as well, and it contains an extensive comment that documents that feature right on top of the feature declaration
There was a problem hiding this comment.
@Veetaha, thank you for the clarification. I see that the bon/Cargo.toml file does contain extensive documentation for the experimental-overwritable feature as you mentioned. I've noted this information for future reviews.
(・ω・)ゞ
✏️ Learnings added
Learnt from: Veetaha
PR: elastio/bon#160
File: bon-macros/Cargo.toml:66-68
Timestamp: 2024-10-27T15:42:41.282Z
Learning: In the `bon` crate, the `bon/Cargo.toml` file contains extensive documentation for the `experimental-overwritable` feature above its feature declaration.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Part of #149
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes