-
Notifications
You must be signed in to change notification settings - Fork 5
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 description to generated packages #128
base: master
Are you sure you want to change the base?
Conversation
⏱ Workflow Timer ⏱ Make sure you keep an eye on build times! One of this project's goals is to keep CI runs under 5 minutes so developers can maintain fast edit-compile-test cycles.
🤖 Beep. Boop. I'm a bot. If you find any issues, please report them to https://github.com/Michael-F-Bryan/workflow-timer. |
crates/wasmer-pack/src/pirita.rs
Outdated
.collect::<Result<Vec<_>, _>>()?; | ||
|
||
Ok(libraries) | ||
} | ||
|
||
fn metadata(fully_qualified_package_name: &str) -> Result<Metadata, Error> { | ||
#[derive(Debug, Serialize, Deserialize)] | ||
struct InternalPackageMeta { |
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.
Not needed anymore. Will remove this.
removed unnused serde imports
74f7cd0
to
bd9d4a1
Compare
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.
Thanks for making this PR @ayys!
I've suggested a slightly different way to get the description. It means we'll need to also modify the code generating pyproject.toml
, but should theoretically be cleaner because we avoid error cases and only do python-specific requirements in the wasmer_pack::py
module.
crates/wasmer-pack/src/pirita.rs
Outdated
#[derive(Debug, Serialize, Deserialize)] | ||
struct InternalPackageMeta { | ||
description: 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 should be safe to remove now.
crates/wasmer-pack/src/pirita.rs
Outdated
/// Retrieves the package description from the given `Manifest` object, | ||
/// If the `wapm` annotation is not found in manifest, returns an empty string. | ||
/// | ||
/// # Arguments | ||
/// | ||
/// * `manifest` - A reference to the `webc::metadata::Manifest` object containing the package metadata. | ||
/// | ||
/// # Returns | ||
/// | ||
/// A string representing the package description, or an empty string if the `wapm` annotation is not found. | ||
fn get_description_from_webc_manifest(manifest: &Manifest) -> String { | ||
let wapm: Option<webc::metadata::annotations::Wapm> = | ||
manifest.package_annotation("wapm").unwrap(); | ||
match wapm { | ||
None => "".to_string(), | ||
Some(wapm) => wapm.description, | ||
} | ||
} |
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 function is probably better written as the following one-liner:
use webc::metadata::annotations::Wapm;
m.package_annotation("wapm")
.ok()
.map(|Wapm { description, .. }| description)
(which can be inlined into `metadata()` if you want)
fn metadata(webc: &WebC<'_>, fully_qualified_package_name: &str) -> Result<Metadata, Error> {
let (unversioned_name, version) = fully_qualified_package_name.split_once('@').unwrap();
let package_name = unversioned_name
.parse()
.context("Unable to parse the package name")?;
let mut metadata = Metadata::new(package_name, version);
if let Ok(Wapm { description, .. }) = webc.manifest.package_annotation("wapm") {
metadata = metadata.with_description(description);
}
Ok(metadata)
}
That way we don't crash when the "wapm"
annotation is missing, and we return Option<String>
rather than using an empty string.
I feel like if PyPI requires a description to be provided, we should generate an error at the top of wasmer_pack::py::generate_python()
or fall back to the empty string by making this description
field required and doing description.as_deref().unwrap_or_default()
here.
What are your thoughts?
Description
Current state:
package.json
(for javascript projects) andpyptoject.toml
(for python projects)don't have a
description
field. This PR adds the field to those files.Note: I added a function
get_description_from_webc_manifest
towasmer-pack/pirita.rs
. This is verysimilar to the
get_package_name_from_manifest
function in webc crate, and should probably belong there. If so, I need guidance moving it there.Context
While trying to publish a python package generated by wasmer-pack to pypi, I ran into an error from
twine.
This was solved when I added the
description
andreadme
fields to my pyproject.toml. This PR itself doesn't solve my problem - I still need the readme field added to pyproject.toml.Checklist
CHANGELOG.md
Fixes #6.