no periods in new contract names #192
Conversation
src/cmd/new.rs
Outdated
| @@ -27,6 +27,10 @@ pub(crate) fn execute<P>(name: &str, dir: Option<P>) -> Result<Option<String>> | |||
| where | |||
| P: AsRef<Path>, | |||
| { | |||
| if name.contains('.') { | |||
There was a problem hiding this comment.
Rather than checking for individual invalid characters, we should rather only allow valid characters e.g. alphanumeric, underscores.
|
@ascjones good call, made that change and removed these invalid character checks. |
src/cmd/new.rs
Outdated
| @@ -27,8 +27,8 @@ pub(crate) fn execute<P>(name: &str, dir: Option<P>) -> Result<Option<String>> | |||
| where | |||
| P: AsRef<Path>, | |||
| { | |||
| if name.contains('-') { | |||
| anyhow::bail!("Contract names cannot contain hyphens"); | |||
| if !name.replace('_', "").chars().all(char::is_alphanumeric) { | |||
There was a problem hiding this comment.
| if !name.replace('_', "").chars().all(char::is_alphanumeric) { | |
| if !name.chars().all(|c| c.is_alphanumeric() || c == '_') { |
I think something like this is more readable (note I haven't checked whether that compiles or not)
There was a problem hiding this comment.
Good idea, probably a tad more efficient too. Made the change and it does indeed compile
| @@ -27,8 +27,8 @@ pub(crate) fn execute<P>(name: &str, dir: Option<P>) -> Result<Option<String>> | |||
| where | |||
| P: AsRef<Path>, | |||
| { | |||
| if name.contains('-') { | |||
| anyhow::bail!("Contract names cannot contain hyphens"); | |||
| if !name.chars().all(|c| c.is_alphanumeric() || c == '_') { | |||
There was a problem hiding this comment.
this allows _ as identifier which is invalid. however, I think this isn't too bad?
There was a problem hiding this comment.
Well spotted. Is that even a valid crate name?
There was a problem hiding this comment.
I just checked and cargo does not immediately put a stop to having _ as crate name but my filesystem definitely does. xD
There was a problem hiding this comment.
Note sure we really need to guard against this. What is your opinion?
to address issue #34