-
Notifications
You must be signed in to change notification settings - Fork 144
Conversation
Hey, thanks for the PR, and sorry for the delay in reacting to it. I just don't feel like I should be deciding whether to include this kind of functionality, only being involved in this project for a rather short time.
That said, personally I like this change. The only improvement I can suggest is mentioning this in README.md too.
@sirwindfield @KodrAus wdyt?
mainrs
commented
Aug 20, 2020
I do like the change! Is it possible that two crates exist with the same name but one uses hyphens, the other dashes? I can only imagine that this can't be the case, as their import names would be the same. This would be the only concern I'd have though.
The crate name isn't necessarily determined by the package name, you can overwrite it in the [lib] section1. I don't actually know how this interacts with log though.
1 https://doc.rust-lang.org/cargo/reference/cargo-targets.html#configuring-a-target
jplatte
commented
Aug 20, 2020
@Gnnng I thought it was obvious so didn't leave another review comment, but the typo was replicated in the assertion a few lines below, you need to update that one too :)
145fcbb to
93995ae
Compare
Gnnng
commented
Aug 20, 2020
My bad, just fixed it and rebased it. Thanks for reviewing it. Any suggestions on what to put in the README.md? It's better if it comes from the maintainers so feel free to amend the PR.
jplatte
commented
Aug 20, 2020
Any suggestions on what to put in the
README.md?
I was thinking something similar to what you wrote for lib.rs.
KodrAus
commented
Jan 11, 2021
This looks good to me! There shouldn't be any conflicts for names. Cargo accepts either - or _ but considers those characters equal for determining the name.
jplatte
commented
Jan 14, 2021
@Gnnng Can you rebase again and add this to the readme? If not I might do it, but no promises.
Gnnng
commented
Jan 14, 2021
Sure, let me do that.
93995ae to
d67d89b
Compare
mainrs
commented
Mar 7, 2021
I can take a closer look an Tuesday, up until then I don't have enough time for a full review :)
@mainrs
mainrs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Tests are there and proper documentation has been added to both the readme and the doc comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So with the crate name also being overridable replacing - by _ is not always the right conversion from package name to crate name (example: rust-s3 where the package is named rust-s3 but the library crate is named s3, not rust_s3).
I guess this still doesn't break anything though, it could just be expected by people that it does more than it actually does 🤷🏼
mainrs
commented
Mar 22, 2021
So with the crate name also being overridable replacing
-by_is not always the right conversion from package name to crate name (example: rust-s3 where the package is namedrust-s3but the library crate is nameds3, notrust_s3).I guess this still doesn't break anything though, it could just be expected by people that it does more than it actually does 🤷🏼
We could add a notice to the README with this example to bring attention to these cases 🤔
Uh oh!
There was an error while loading. Please reload this page.
This pull request improves the user experience when package names contain hyphens. Users can use either the original form (
my-app) or the canonical form (my_app).See more about name using hyphens in this RFC.
Fixes #197