Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
JaviSoto
force-pushed
the
dev/javi/analytics-config
branch
from
7602905 to
bc086d6
Compare
|
@codex review |
JaviSoto
force-pushed
the
dev/javi/analytics-config
branch
from
bc086d6 to
c35195c
Compare
JaviSoto
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Could we make this a bit more complex? I expect we will have multiple fields in there in the end so could we have something like: And btw we are not allowed to enable it by default in some juridiction so we must check with legal (I already sent the request but no answer yet) |
|
@jif-oai rebased and updated as per your suggestion! How does it look? |
JaviSoto
force-pushed
the
dev/javi/analytics-config
branch
from
ee05a22 to
40c82b9
Compare
test(config): cover [analytics].enabled in fixture
JaviSoto
force-pushed
the
dev/javi/analytics-config
branch
from
40c82b9 to
03f7260
Compare
| .as_ref() | ||
| .and_then(|a| a.enabled) | ||
| .or(cfg.analytics.as_ref().and_then(|a| a.enabled)) | ||
| .unwrap_or(true), |
There was a problem hiding this comment.
I guess this has been validated with legal right?
There was a problem hiding this comment.
Yup, I'll ping you in Slack with more details
codex-rs/core/src/config/types.rs
Outdated
| /// Analytics settings loaded from config.toml. Fields are optional so we can apply defaults. | ||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default)] | ||
| pub struct AnalyticsConfigToml { | ||
| /// When `false`, disables analytics across Codex product surfaces in this machine. |
There was a problem hiding this comment.
| /// When `false`, disables analytics across Codex product surfaces in this machine. | |
| /// When `false`, disables analytics across Codex product surfaces in this profile. |
| approval_policy = "on-failure" | ||
|
|
||
| [profiles.zdr.analytics] | ||
| enabled = false |
There was a problem hiding this comment.
Can you add a test to make sure this work?
There was a problem hiding this comment.
It's there! see test_precedence_fixture_with_zdr_profile below, it checks that analytics is false
2 participants
No description provided.