Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Support kebab-case names in parse sepc#177

Open
Gnnng wants to merge 3 commits intorust-cli:main from
Gnnng:support-kebab-case
Open

Support kebab-case names in parse sepc #177
Gnnng wants to merge 3 commits intorust-cli:main from
Gnnng:support-kebab-case

Conversation

@Gnnng
Copy link

@Gnnng Gnnng commented Aug 14, 2020
edited by epage
Loading

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

Copy link
Contributor

jplatte commented Aug 20, 2020
edited
Loading

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?

Copy link
Contributor

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.

Copy link
Contributor

jplatte commented Aug 20, 2020
edited
Loading

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

Copy link
Contributor

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 :)

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Collaborator

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 reacted with thumbs up emoji

Copy link
Contributor

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.

Copy link
Author

Gnnng commented Jan 14, 2021

Sure, let me do that.

Copy link
Contributor

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 :)

Copy link
Contributor

@mainrs mainrs left a 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.

Copy link
Contributor

@jplatte jplatte left a comment
edited
Loading

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 🤷🏼

Copy link
Contributor

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 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 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jplatte jplatte jplatte approved these changes

@mainrs mainrs mainrs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Handle hyphens internally

Comments

AltStyle によって変換されたページ (->オリジナル) /