-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Allow the edition specifier to be an integer #12301
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -979,6 +979,22 @@ impl StringOrVec { | |
} | ||
} | ||
|
||
#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)] | ||
#[serde(untagged, expecting = "expected a string or an integer")] | ||
pub enum StringOrI64 { | ||
String(String), | ||
I64(i64), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have tried an u32 at the start and it didn't work, as in it compiled but the tests as added by this PR failed. The problem was I think that serde represents toml integers via i64 in its data model. In the end I don't think it matters that much: a negative number will still give an error one way or another. |
||
} | ||
|
||
impl Display for StringOrI64 { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
match self { | ||
StringOrI64::String(s) => f.write_str(s), | ||
StringOrI64::I64(v) => write!(f, "{v}"), | ||
} | ||
} | ||
} | ||
|
||
#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)] | ||
#[serde(untagged, expecting = "expected a boolean or a string")] | ||
pub enum StringOrBool { | ||
|
@@ -1262,6 +1278,50 @@ impl TomlWorkspaceDependency { | |
//. This already has a `Deserialize` impl from version_trim_whitespace | ||
type MaybeWorkspaceSemverVersion = MaybeWorkspace<semver::Version, TomlWorkspaceField>; | ||
|
||
type MaybeWorkspaceStringOrI64 = MaybeWorkspace<StringOrI64, TomlWorkspaceField>; | ||
impl<'de> de::Deserialize<'de> for MaybeWorkspaceStringOrI64 { | ||
fn deserialize<D>(d: D) -> Result<Self, D::Error> | ||
where | ||
D: de::Deserializer<'de>, | ||
{ | ||
struct Visitor; | ||
|
||
impl<'de> de::Visitor<'de> for Visitor { | ||
type Value = MaybeWorkspaceStringOrI64; | ||
|
||
fn expecting(&self, f: &mut fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { | ||
f.write_str("a string, integer or workspace") | ||
} | ||
|
||
fn visit_i64<E>(self, value: i64) -> Result<Self::Value, E> | ||
where | ||
E: de::Error, | ||
{ | ||
let int = de::value::I64Deserializer::new(value); | ||
StringOrI64::deserialize(int).map(MaybeWorkspace::Defined) | ||
} | ||
|
||
fn visit_string<E>(self, value: String) -> Result<Self::Value, E> | ||
where | ||
E: de::Error, | ||
{ | ||
let string = de::value::StringDeserializer::new(value); | ||
StringOrI64::deserialize(string).map(MaybeWorkspace::Defined) | ||
} | ||
|
||
fn visit_map<V>(self, map: V) -> Result<Self::Value, V::Error> | ||
where | ||
V: de::MapAccess<'de>, | ||
{ | ||
let mvd = de::value::MapAccessDeserializer::new(map); | ||
TomlWorkspaceField::deserialize(mvd).map(MaybeWorkspace::Workspace) | ||
} | ||
} | ||
|
||
d.deserialize_any(Visitor) | ||
} | ||
} | ||
|
||
type MaybeWorkspaceString = MaybeWorkspace<String, TomlWorkspaceField>; | ||
impl<'de> de::Deserialize<'de> for MaybeWorkspaceString { | ||
fn deserialize<D>(d: D) -> Result<Self, D::Error> | ||
|
@@ -1501,7 +1561,7 @@ impl WorkspaceInherit for TomlWorkspaceField { | |
#[derive(Deserialize, Serialize, Clone, Debug)] | ||
#[serde(rename_all = "kebab-case")] | ||
pub struct TomlPackage { | ||
edition: Option<MaybeWorkspaceString>, | ||
edition: Option<MaybeWorkspaceStringOrI64>, | ||
rust_version: Option<MaybeWorkspaceString>, | ||
name: InternedString, | ||
#[serde(deserialize_with = "version_trim_whitespace")] | ||
|
@@ -1583,7 +1643,7 @@ pub struct InheritableFields { | |
license_file: Option<String>, | ||
repository: Option<String>, | ||
publish: Option<VecStringOrBool>, | ||
edition: Option<String>, | ||
edition: Option<StringOrI64>, | ||
badges: Option<BTreeMap<String, BTreeMap<String, String>>>, | ||
exclude: Option<Vec<String>>, | ||
include: Option<Vec<String>>, | ||
|
@@ -1620,7 +1680,7 @@ impl InheritableFields { | |
("package.categories", categories -> Vec<String>), | ||
("package.description", description -> String), | ||
("package.documentation", documentation -> String), | ||
("package.edition", edition -> String), | ||
("package.edition", edition -> StringOrI64), | ||
("package.exclude", exclude -> Vec<String>), | ||
("package.homepage", homepage -> String), | ||
("package.include", include -> Vec<String>), | ||
|
@@ -1728,7 +1788,7 @@ impl TomlManifest { | |
.edition | ||
.as_ref() | ||
.and_then(|e| e.as_defined()) | ||
.map(|e| Edition::from_str(e)) | ||
.map(|e| Edition::from_str(&e.to_string())) | ||
.unwrap_or(Ok(Edition::Edition2015)) | ||
.map(|e| e.default_resolve_behavior()) | ||
})?; | ||
|
@@ -2040,9 +2100,12 @@ impl TomlManifest { | |
let edition = if let Some(edition) = package.edition.clone() { | ||
let edition: Edition = edition | ||
.resolve("edition", || inherit()?.edition())? | ||
.to_string() | ||
.parse() | ||
.with_context(|| "failed to parse the `edition` key")?; | ||
package.edition = Some(MaybeWorkspace::Defined(edition.to_string())); | ||
package.edition = Some(MaybeWorkspace::Defined(StringOrI64::String( | ||
edition.to_string(), | ||
))); | ||
edition | ||
} else { | ||
Edition::Edition2015 | ||
|
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.
Other areas I suspect we should update if we move forward with this
cargo new
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.
@epage We shouldn't update
cargo new
until 2024, to avoid MSRV implications.Uh oh!
There was an error while loading. Please reload this page.
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.
There has been some discussion on this in #12301 (review) and #12301 (comment) . I agree that
cargo new
should be updated only well into the 2023 edition, or even later, not just for MSRV but also for better error messages from cargos that don't support the 2023 edition.