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

Allow snake case components #354

Merged
merged 2 commits into from
Jan 22, 2023

Conversation

jclmnop
Copy link
Contributor

@jclmnop jclmnop commented Jan 22, 2023

Couldn't find any guides for external contributor PRs, so I'm winging this a bit.

What?

  • Allow snake_case functions to be used for components
  • Add a unit test
  • Update docs for #[component] macro (very minor change)

How?

Added a little function which uses the convert_case crate to check whether item.sig.ident is snake_case, and if so it converts the ident to PascalCase (referred to as camelCase in the docs, but I don't think the distinction matters too much because most people refer to it as that anyway) which is later used to generate the Component and ComponentProps structs.

use convert_case::{Case::{Snake, Pascal}, Casing};

// . . .

fn convert_from_snake_case(name: &Ident) -> Ident {
    let name_str = name.to_string();
    if !name_str.is_case(Snake) {
        name.clone()
    } else {
        Ident::new(&*name_str.to_case(Pascal), name.span().clone())
    }
}

The reason it only bothers to convert from snake_case is because that's the convention for naming functions in Rust, and converting from other types of casing might lead to some extra edge cases.

Why?

  • Leaving open the option to use snake_case function names for components (which are then converted to CamelCase to avoid the structs being confused for html elements) seems reasonable to me; sometimes it's the little things such as having to edit your IDE/cargo fmt config or dealing with a bunch of warnings when using a specific framework that can put people off.
  • When it comes to attribute macros which generate a struct from a function, I often prefer them to have different casing because it makes it quite clear (even if you don't understand what the macro really does) that it's generating a struct from your function, and you're not actually passing a function around whenever you use it. This is entirely a matter of preference, which is why I think it's a good idea to give people the option.
  • Reduces potential for new users of the framework to encounter errors. Time spent figuring out why your snake_case function doesn't want to become a component is time that could have been spent learning something more important within the framework.

Improvements/Limitations

  • I'm unsure of how convert_case handles acronyms (e.g. does_HTML_stuff), so in cases like that it might not recognise the function name as snake_case and no conversion will be done.
  • Leading or trailing underscores might be removed when converting to CamelCase (_my_component -> MyComponent, my_component_ -> MyComponent).
  • Could be some other edge cases, but I didn't want to come in guns blazing and add a bunch of unit tests for such a small function.

I am of course not as well versed in the internals of Leptos as the maintainers, so perhaps there's some reason that this can't be implemented. If that's the case then feel free to reject this PR, but I'd still be interested in understanding why it won't work, just because I'm curious.

@jclmnop jclmnop force-pushed the feat/allow-snake-case-components branch from 50e45d5 to 39cddfc Compare January 22, 2023 17:13
@jclmnop
Copy link
Contributor Author

jclmnop commented Jan 22, 2023

Run davidB/rust-cargo-make@v1
[2](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:2)
  with:
[3](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:3)
    version: latest
[4](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:4)
    fallback_version: 0.36.3
[5](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:5)
    github_token: ***
[6](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:6)
  env:
[7](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:7)
    CARGO_TERM_COLOR: always
[8](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:9)
search latest version of cargo-make
[9](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:10)
installing cargo-make 0.36.4 ...
[10](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:11)
downloading https://github.com/sagiegurari/cargo-make/releases/download/0.36.4/cargo-make-v0.36.4-x86_64-unknown-linux-musl.zip
[11](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:12)
Error: Error: Unexpected HTTP response: 404
[12](https://github.com/leptos-rs/leptos/actions/runs/3980710499/jobs/6824377677#step:4:13)
Error: Error: Unexpected HTTP response: 404

Not too sure why it's failing to install cargo-make during the workflow, I haven't touched any of the CI stuff.

@gbj
Copy link
Collaborator

gbj commented Jan 22, 2023

Hi! I just saw that and I don't know either. Weird and definitely not on you.

This seems fine to me. It doesn't seem like it should cause any issue with the view macro as it's still converting to PascalCase for the component name and props. I will try to figure out the CI situation and merge this once I get it passing.

@jclmnop
Copy link
Contributor Author

jclmnop commented Jan 22, 2023

Hi! I just saw that and I don't know either. Weird and definitely not on you.

Good to know, I was worried I'd somehow broken something.

This seems fine to me. It doesn't seem like it should cause any issue with the view macro as it's still converting to PascalCase for the component name and props. I will try to figure out the CI situation and merge this once I get it passing.

I ran cargo make ci on My Machine™ and it all worked okay, including the ssr_test_with_snake_case_components test, so hopefully once the CI is sorted it'll pass without any issues.

@jclmnop jclmnop changed the title Feat/allow snake case components Allow snake case components Jan 22, 2023
@gbj gbj merged commit fd6e637 into leptos-rs:main Jan 22, 2023
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