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

Attempt to improve the documentation #45

Merged
merged 9 commits into from
Nov 17, 2023
Merged

Conversation

cecton
Copy link
Member

@cecton cecton commented Nov 9, 2023

No description provided.

@cecton cecton marked this pull request as ready for review November 17, 2023 08:53
@cecton
Copy link
Member Author

cecton commented Nov 17, 2023

I think this example is now more reflecting the actual usage. What do you think?

// In the host library source code:

use implicit_clone::ImplicitClone;
use implicit_clone::unsync::{IArray, IString};

macro_rules! html_input {
    (<input $(type={$ty:expr})? $(name={$name:expr})? $(value={$value:expr})?>) => {{
        let mut input = Input::new();
        $(input.type = $ty.into();)*
        $(input.name.replace($name.into());)*
        $(input.value.replace($value.into());)*
        input
    }}
}

#[derive(Clone)]
pub struct Input {
    ty: IString,
    name: Option<IString>,
    value: Option<IString>,
}

impl ImplicitClone for Input {}

impl Input {
    pub fn new() -> Self {
        Self {
            ty: IString::Static("text"),
            name: None,
            value: None,
        }
    }
}

impl std::fmt::Display for Input {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "<input type=\"{}\"", self.ty)?;
        if let Some(name) = self.name.as_ref() {
            write!(f, " name=\"{}\"", name)?;
        }
        if let Some(value) = self.value.as_ref() {
            write!(f, " value=\"{}\"", value)?;
        }
        write!(f, ">")
    }
}

// In the user's source code:

fn component(age: &IString) -> IArray<Input> {
    // `age` is implicitly cloned to the 2 different inputs
    let input1 = html_input!(<input name={"age"} value={age}>);
    let input2 = html_input!(<input name={"age"} value={age}>);

    IArray::from(vec![input1, input2])
}

let age = IString::from(20.to_string());
let output = component(&age);
let output_str = output
    .iter()
    .map(|x| x.to_string())
    .collect::<Vec<_>>()
    .join("");

assert_eq!(
    output_str,
    r#"<input type="text" name="age" value="20"><input type="text" name="age" value="20">"#,
);

@kirillsemyonkin
Copy link
Collaborator

Yes the main point would be that the clone happens implicitly (here using .into()?), so that user would have easier time typing out similar lines without having to think about the moves/lifetimes.

If that example-test passes, and does what I just mentioned above, then any such example will be very good.

IArray::from(vec![input1, input2])

Tiny nitpick/question, should IArray::from([input1, input2]) be used instead? (is it possible?)

Comment on lines -220 to -221
host_library!(&NonImplicitCloneType);
// `host_library!(NonImplicitCloneType)` doesn't compile
Copy link
Member Author

Choose a reason for hiding this comment

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

@kirillsemyonkin static-assertions actually has a macro to test that a type does not implement something. So we can actually test those cases

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be a good idea; won't hurt at least

@cecton
Copy link
Member Author

cecton commented Nov 17, 2023

Tiny nitpick/question, should IArray::from([input1, input2]) be used instead? (is it possible?)

I asked myself the same question but not it doesn't work. But it's actually very logical. IArray has only 3 variants:

  • static slices (not usable here because the inner values must be static too)
  • single item ([T; 1], not our case here, we have 2)
  • Rc<[T]>: ah that's our case! but you still need to allocate and the easiest way is to come from a vec anyway

@cecton
Copy link
Member Author

cecton commented Nov 17, 2023

Yes the main point would be that the clone happens implicitly (here using .into()?),

Yes exactly, the into is the magic here. I think I will put back the setter functions to make it more obvious.

@cecton
Copy link
Member Author

cecton commented Nov 17, 2023

Ok I think now it's perfect but I don't want you to feel forced on my idea.

  • the comments with the "not compile" things are now being actually tested
  • the documentation now has a (hopefully) good example, very close to Yew which is actual real life example (especially useful for those who don't understand the description)
  • the tests lose their "doc as example" aspect but it's moved to the crate's doc so that is okay

@kirillsemyonkin
Copy link
Collaborator

  • the tests lose their "doc as example" aspect

They can't. Even if you make them a bit less reader-friendly, it does not change the fact that tests document code.

  • the comments with the "not compile" things are now being actually tested

It was like my first time writing tests in OSS space actually. So if you were able to improve this, I'm just simply glad that my incompetence was solved at some point.

I don't want you to feel forced on my idea

I mean, ideally both code tests and doc would be aiding users at everything, but I do think having it easily visible in the doc is a good idea. Everyone trying to change the code and having a discussion is good, we're all trying to work in the best interests of the project, each other and the users after all.

@cecton cecton merged commit a3636d8 into yewstack:main Nov 17, 2023
12 checks passed
@cecton cecton deleted the improve-doc branch November 17, 2023 11:23
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