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

Refactor: use camino utf8-path types where applicable #59

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

ian-h-chamberlain
Copy link
Member

This is a refactor in preparation for #58 (which I'll need to rebase after this is merged).

I think the changes probably should have no effect — basically this just enforces all paths must be UTF-8. Since we already used String all over the place I think that was already kinda required, but now it should be reflected in the types we are using too. Otherwise, I just did some minor cleanup. camino was already a transitive dependency via cargo_metadata so no additional code to compile there.

One behavior change: all author names are included (joined by ", ") instead of just the first one, if there's more than one listed for the package.

@ian-h-chamberlain ian-h-chamberlain self-assigned this Jun 8, 2024
@ian-h-chamberlain ian-h-chamberlain requested a review from a team as a code owner June 8, 2024 05:50
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@Meziu Meziu left a comment

Choose a reason for hiding this comment

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

I'm fine with the change, it doesn't change anything semantically and the dependency isn't an addition to the (already quite large) tree.

let author = if self.authors.is_empty() {
String::from("Unspecified Author") // as standard with the devkitPRO toolchain
} else {
self.authors.join(", ")
Copy link
Member

Choose a reason for hiding this comment

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

Originally, I believe I made the choice of including only the first author because there was the slight risk that the combined name of all authors might exceed the SMDH limitations (which is a lengthy 128 bytes in UTF-16 https://www.3dbrew.org/wiki/SMDH#Format, but will never be shown in its entirety if it gets this long).

I guess it doesn't really matter either way, since the authors field is more-or-less getting abandoned by the Rust community (it's not even shown on crates.io anymore). I'm fine with the change.

Copy link
Member Author

@ian-h-chamberlain ian-h-chamberlain Jun 9, 2024

Choose a reason for hiding this comment

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

Ah, hmm... from looking at the smdhtool implementation, it seems like anything longer will just get truncated to 0x40 characters anyway: https://github.com/devkitPro/3dstools/blob/master/src/smdhtool.cpp#L423-L429 — manually testing it seems to work as expected too.

We could add a length check of our own and panic if the joined string is too long, but it would require calculating the UTF-16 length of the input UTF-8 string, so I think just letting it get truncated seems fine. Once #58 is implemented users will also be able to override the author string to their liking without needing to modify Cargo.toml's package.authors field.

src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Address review comments and minor cleanup
@Meziu Meziu merged commit fb32005 into master Jun 13, 2024
3 checks passed
@ian-h-chamberlain ian-h-chamberlain deleted the feature/refactor-paths-romfs branch June 13, 2024 12:45
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