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

UPPER CASE, lower case and Title Case #803

Closed
jonathanpallant opened this issue Feb 1, 2024 · 7 comments · Fixed by #805
Closed

UPPER CASE, lower case and Title Case #803

jonathanpallant opened this issue Feb 1, 2024 · 7 comments · Fixed by #805

Comments

@jonathanpallant
Copy link

In Rust, we typically use TitleCase for types (like struct Uart0 { .. .}), and lower_case for module names (like pac::uart0), method/function names (like my_uart.baudrate()), and field names within structs (like my_uart.baudrate). Things that are UPPER_CASE are typically only static variables and constants.

The code generated by svd2rust currently uses a lot of UPPER_CASE names for types, and also for the field names in struct Peripherals. I think this can be confusing for a newcomer to Embedded Rust.

  • Does anyone recall the reason for using UPPER_CASE here?
  • Is there any appetite for changing the output to follow a more typical approach for capitalising things?
@burrbull
Copy link
Member

burrbull commented Feb 1, 2024

You can specify it by ident_formats.peripheral.case = pascal in config. See #794

@jonathanpallant
Copy link
Author

That's cool. But why is the default the way it is?

@burrbull
Copy link
Member

burrbull commented Feb 1, 2024

I don't have answer.
I think there is many factors you should consider: rust style, datasheet (RM) style, real names in SVD.
Any of variants like: Don't change (leave as in SVD), make constant case, make pascal case, etc. have downsides.
I think upper (constant) case was selected by @japaric just because he was working with STM32 when started the project.

@Emilgardis
Copy link
Member

Maybe it's worth looking into changing the default to PascalCase, I can't think of any reasons to not do it except that it's a breaking change

@jonathanpallant
Copy link
Author

jonathanpallant commented Feb 2, 2024

I would support changing the default - it would make teaching easier... (see https://github.com/ferrous-systems/rust-training/blob/main/training-slides/src/pac-svd2rust.md for my slides on this topic).

Anyone who objected to the massive diff next time they rebuild can add a config file to put it back as it was, right?

Also perhaps the config file can help when it comes to removing (or not) the singleton objects.

@burrbull
Copy link
Member

burrbull commented Feb 2, 2024

We should also split option for peripheral types and peripheral singletons. Now it is one option.

And maybe modules? (Not sure)

@burrbull
Copy link
Member

burrbull commented Feb 2, 2024

🤦
There is also typos.

svd2rust/src/config.rs

Lines 156 to 159 in 869d34f

pub fn scake_case(mut self) -> Self {
self.case = Some(Case::Pascal);
self
}

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 a pull request may close this issue.

3 participants