-
Notifications
You must be signed in to change notification settings - Fork 128
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
Support kebab-case names in parse sepc #177
base: main
Are you sure you want to change the base?
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 @sirwindfield @KodrAus wdyt? |
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 ¹ https://doc.rust-lang.org/cargo/reference/cargo-targets.html#configuring-a-target |
@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
My bad, just fixed it and rebased it. Thanks for reviewing it. Any suggestions on what to put in the |
I was thinking something similar to what you wrote for |
This looks good to me! There shouldn't be any conflicts for names. Cargo accepts either |
@Gnnng Can you rebase again and add this to the readme? If not I might do it, but no promises. |
Sure, let me do that. |
Co-authored-by: Jonas Platte <[email protected]>
93995ae
to
d67d89b
Compare
I can take a closer look an Tuesday, up until then I don't have enough time for a full review :) |
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 🤷🏼
We could add a notice to the README with this example to bring attention to these cases 🤔 |
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