-
Notifications
You must be signed in to change notification settings - Fork 26
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
add commit hashes to git sources #123
Conversation
Thank you! I will be traveling for the next three days, so I will review this on the 27th. Is that OK? |
I have fixed one thing that slipped from my previous commit. I worry however that this might not be enough to pass all the checks run by GithHub. I have previously on my own machine noticed that the test runs seemed to be nondeterministic (failure at |
"Source": { | ||
"description": "Serializes to \"git\", \"local\", \"crates.io\" or \"registry\". Designed to be extensible with other revision control systems, etc.", | ||
"oneOf": [ | ||
{ | ||
"type": "string", | ||
"enum": [ | ||
"CratesIo", | ||
"Git", | ||
"Local", | ||
"Registry" | ||
] | ||
}, | ||
{ | ||
"type": "object", | ||
"required": [ | ||
"Other" | ||
], | ||
"properties": { | ||
"Other": { | ||
"type": "string" | ||
} | ||
}, | ||
"additionalProperties": false | ||
} | ||
] |
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.
It is not obvious at first glance but the new schema should be backwards compatible and as far as I am aware even more correct as the values for enum
keyword used here are not exactly right (e.g. de-serialization expects crates.io
rather than CratesIo
).
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.
Yes, the schema should be re-generated.
The mismatch between the documentation and the generated schema is a bug, and will need to be fixed, but that doesn't have to be done in this PR.
{ | ||
"description": "\"local\"", | ||
"type": "string" | ||
}, | ||
{ | ||
"description": "\"registry\"", | ||
"type": "string" | ||
}, |
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.
Providing specific accepted values using proper keywords turned out to be too difficult and probably not worth the complexity because of how schemars
operates. This is not my first issue with schemars
and the crate is not very actively maintained so I must admit I did not even try to formulate the exact problem that manifested here into a bug report on their repository.
#[serde(with = "compact_enum_variant")] | ||
#[cfg_attr( | ||
feature = "schema", | ||
schemars(schema_with = "compact_enum_variant::schema::<Source, GitSource>",) | ||
)] | ||
Git(GitSource), |
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.
This specific fragment changes the Git
variant to use the representation specified in the compact_enum_variant.rs
file. The attributes did became a little bit complex because of that but besides most of the complexity should be hidden away in that file and in future a similar trick can be used to extend any other enum variant while keeping it all backwards compatible with the way the unit variants are serialized.
That is, a change from:
#[derive(Serialize, Deserialize)]
enum MyEnum{
V1,
}
to:
#[derive(Serialize, Deserialize)]
#[serde(untagged)]
enum MyEnum {
#[serde(with = "compact_enum_variant")]
V1(V1Struct),
}
/// And some impls ... {}
Should be backwards compatible.
impl From<Source> for String { | ||
/// Provides a lossy conversion for variants with values different than the default | ||
fn from(s: Source) -> String { | ||
match s { | ||
Source::CratesIo => "crates.io".to_owned(), | ||
Source::Git => "git".to_owned(), | ||
Source::Git(_) => "git".to_owned(), | ||
Source::Local => "local".to_owned(), | ||
Source::Registry => "registry".to_owned(), | ||
Source::Other(string) => string, | ||
} | ||
} | ||
} |
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.
This implementation along with From<&str> for Source
make me feel the most conflicted. They are a part of publicly exposed API so changing their behavior could become problematic. For some reason I feel like I shouldn't just use serde_json and parse/serialize the value for git variant but I am not sure if the feeling is justified.
Thank you for the PR! I will take a more thorough look in the next few days. Do you have a deadline for this PR to be merged? |
The deadline would be actually tomorrow but I can ask for extension. I am
pretty sure I would get one but I don't know how much of an extension it
would be.
…On Tue, 27 Jun 2023, 17:10 Shnatsel, ***@***.***> wrote:
Thank you for the PR! I will take a more thorough look in the next few
days. Do you have a deadline for this PR to be merged?
—
Reply to this email directly, view it on GitHub
<#123 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMP4ZPZIYNIJ4FB3TK6DHQTXNLZXXANCNFSM6AAAAAAZRBKWQU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I see. The timing of my trip was really inconvenient. I'll get this reviewed and merged later tonight then. |
Don't worry. If I miss it a day or two it won't be a big issue. Since with
the kind of project I am doing the professor has nothing to review himself
he rather won't mind.
…On Tue, 27 Jun 2023, 18:22 Shnatsel, ***@***.***> wrote:
I see. The timing of my trip was really inconvenient. I'll get this
reviewed and merged later tonight then.
—
Reply to this email directly, view it on GitHub
<#123 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMP4ZP3OXJXGYUFZJVFXYMLXNMCFLANCNFSM6AAAAAAZRBKWQU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
//! ``` | ||
//! | ||
//! Changing a unit variant of an enum to wrap a type and use this module for | ||
//! the serialization can be made to be a backwards compatible change. |
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.
Nice documentation!
#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] | ||
pub enum VariantRepr<S: Into<String>, ENUM, INNER> { | ||
// Short stringly-typed representation of the enum variant - a label - | ||
// which assumes all fields are set to their defualts. |
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.
// which assumes all fields are set to their defualts. | |
// which assumes all fields are set to their defaults. |
Typo
|
||
#[test] | ||
#[should_panic] | ||
fn fail_deserializing_invalid_git_source_variant() { |
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.
Great to see test coverage for both success and failure cases!
I might investigate later whether I want to keep around the untagged enum magic. If there is a way to alter the format to keep the deserialization simple and stupid, it might be worth changing the spec to accommodate that. But this is a nice, general, united-tested solution within the confines of the current spec. Great work, thank you! |
Thank you!
I was honestly quite ready to just put a `git_rev` field directly into the
`Package` if this was decided to be too complex. It was however sticking my
eyes hard enough that I decided to try something else first. I imagine it
could be desirable to be able to extend e.g. `Local` case in the future, as
the dependencies are all available while building with cargo-auditable the
tool could try to include some other metadata, one that is not stored in
cargo's lockfile. This made me wish for a solution that can be hierarchical
and thanks to that still readable.
I'll subscribe to issues and watch out if there are any caused by the
changes for at least this summer.
…On Wed, 28 Jun 2023, 22:07 Shnatsel, ***@***.***> wrote:
I might investigate later whether I want to keep around the untagged enum
magic. If there is a way to alter the format to keep the deserialization
simple and stupid, it might be worth changing the spec to accommodate that.
But this is a nice, general, united-tested solution within the confines of
the current spec.
Great work, thank you!
—
Reply to this email directly, view it on GitHub
<#123 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMP4ZP3HUZBJCJFZUDP5BUTXNSFITANCNFSM6AAAAAAZRBKWQU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This would close the issue #122 . The suggested approach to serialization of git sources might seem over-engineered but my hope while working on it was to keep it all backwards compatible and to establish a good way of adding more of similar source parameters if needed.
At this very moment I am somewhat sleep deprived so I will probably get back here later to try and explain more if needed.