Skip to content

Update cargo to 2024 edition #842

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

Merged
merged 1 commit into from
May 7, 2025
Merged

Conversation

jcoens-openai
Copy link
Contributor

@jcoens-openai jcoens-openai commented May 6, 2025

Some effects of this change:

  • New formatting changes across many files. No functionality changes should occur from that.
  • Calls to set_env are considered unsafe, since this only happens in tests we wrap them in unsafe blocks

Copy link

github-actions bot commented May 6, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@jcoens-openai
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request May 6, 2025
Copy link
Collaborator

@bolinfest bolinfest left a comment

Choose a reason for hiding this comment

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

Maybe mention the unsafe bit in the PR body since that's slightly unexpected?

std::env::set_var("OPENAI_REQUEST_MAX_RETRIES", "2");
std::env::set_var("OPENAI_STREAM_MAX_RETRIES", "2");
//
// NOTE: Starting with the 2024 edition `std::env::set_var` is `unsafe`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a big fan of this change!

@@ -1,7 +1,7 @@
[package]
name = "codex-cli"
version = { workspace = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

outside the scope of this PR, but I guess all of our Cargo.toml files should have version = { workspace = true }?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that'd be a bit cleaner

@bolinfest
Copy link
Collaborator

Also, can you please rebase past #843 and make sure the common crate introduced in that PR gets this new 2024 treatment, as well?

@jcoens-openai jcoens-openai force-pushed the dev/jcoens/cargo_update_edition branch from b6cf095 to 6fdb30e Compare May 7, 2025 03:01
@jcoens-openai jcoens-openai marked this pull request as ready for review May 7, 2025 15:18
@jcoens-openai jcoens-openai requested a review from bolinfest May 7, 2025 15:22
@jcoens-openai jcoens-openai force-pushed the dev/jcoens/cargo_update_edition branch from 6fdb30e to 8d70a56 Compare May 7, 2025 15:24
Copy link
Collaborator

@bolinfest bolinfest left a comment

Choose a reason for hiding this comment

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

Nice work! Do you mind updating the PR body to call out:

  • Upgrading to 2024 caused formatting changes (is that accurate)?
  • Introduced the use of unsafe for setenv calls, but those are limited to tests?

@jcoens-openai jcoens-openai merged commit 8a89d3a into main May 7, 2025
8 checks passed
@jcoens-openai jcoens-openai deleted the dev/jcoens/cargo_update_edition branch May 7, 2025 15:37
@github-actions github-actions bot locked and limited conversation to collaborators May 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants