Skip to content
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

adding most of the missing fields in CfProperties #233

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seeekr
Copy link

@seeekr seeekr commented Oct 8, 2022

There's still a few holes, but it's good enough for my own purposes. Would be nice if someone could review this and leave me some feedback :)
Also there's a review comment in there that I'd like to discuss!

…riate Rust + bindgen representations; plus some remaining TODOs and a review comment on related code
#[wasm_bindgen]
#[derive(Clone, Copy)]
pub enum ResizeFit {
ScaleDown,
Copy link
Contributor

@jakubadamw jakubadamw Oct 9, 2022

Choose a reason for hiding this comment

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

Values of the enum types (ResizeFit, ResizeFormat, ResizeMetadata, ResizeOnerror) will need to be converted with their target naming scheme (kebab scheme) accounted for, essentially like it's done above with PolishConfig.

For example, ScaleDown should be serialised into the string scale-down.

I see the existing &str conversions in the file are spelled out explicitly, which is a bit of a shame, since that means a lot of repetition. I would use strum::IntoStaticStr, but I don't know if the maintainers are okay with pulling in that dependency.

@jakubadamw
Copy link
Contributor

@seeekr if you're keen on continuing with this PR, I have proposed some changes in https://github.com/jakubadamw/workers-rs/commits/add-cf-properties. 🙂

I tested the branch (well, some of the properties, at least) in a personal project. Seems to work fine! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants