-
-
Notifications
You must be signed in to change notification settings - Fork 808
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
Suggest valid project names on InvalidProjectName #3913
Changes from all commits
39f2e55
9f83045
014a844
2e9bfd4
677f0b3
cb5ef3f
9858254
c999e7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,7 +1,7 @@ | ||||||
use camino::{Utf8Path, Utf8PathBuf}; | ||||||
use clap::ValueEnum; | ||||||
use gleam_core::{ | ||||||
erlang, | ||||||
erlang, error, | ||||||
error::{Error, FileIoAction, FileKind, InvalidProjectNameReason}, | ||||||
parse, Result, | ||||||
}; | ||||||
|
@@ -203,16 +203,7 @@ jobs: | |||||
|
||||||
impl Creator { | ||||||
fn new(options: NewOptions, gleam_version: &'static str) -> Result<Self, Error> { | ||||||
let project_name = if let Some(name) = options.name.clone() { | ||||||
name | ||||||
} else { | ||||||
get_foldername(&options.project_root)? | ||||||
} | ||||||
.trim() | ||||||
.to_string(); | ||||||
|
||||||
validate_name(&project_name)?; | ||||||
|
||||||
let project_name = get_valid_project_name(options.name.clone(), &options.project_root)?; | ||||||
let root = get_current_directory()?.join(&options.project_root); | ||||||
let src = root.join("src"); | ||||||
let test = root.join("test"); | ||||||
|
@@ -349,16 +340,99 @@ fn validate_name(name: &str) -> Result<(), Error> { | |||||
name: name.to_string(), | ||||||
reason: InvalidProjectNameReason::GleamReservedModule, | ||||||
}) | ||||||
} else if !regex::Regex::new("^[a-z][a-z0-9_]*$") | ||||||
.expect("new name regex could not be compiled") | ||||||
} else if regex::Regex::new("^[a-z][a-z0-9_]*$") | ||||||
.expect("failed regex to match valid name format") | ||||||
.is_match(name) | ||||||
{ | ||||||
Ok(()) | ||||||
} else if regex::Regex::new("^[a-zA-Z][a-zA-Z0-9_]*$") | ||||||
.expect("failed regex to match valid but non-lowercase name format") | ||||||
.is_match(name) | ||||||
{ | ||||||
Err(Error::InvalidProjectName { | ||||||
name: name.to_string(), | ||||||
reason: InvalidProjectNameReason::Format, | ||||||
reason: InvalidProjectNameReason::FormatNotLowercase, | ||||||
}) | ||||||
} else { | ||||||
Ok(()) | ||||||
Err(Error::InvalidProjectName { | ||||||
name: name.to_string(), | ||||||
reason: InvalidProjectNameReason::Format, | ||||||
}) | ||||||
} | ||||||
} | ||||||
|
||||||
fn suggest_valid_name(invalid_name: &str, reason: &InvalidProjectNameReason) -> Option<String> { | ||||||
match reason { | ||||||
InvalidProjectNameReason::GleamPrefix => match invalid_name.strip_prefix("gleam_") { | ||||||
Some(stripped) if invalid_name != "gleam_" => { | ||||||
let suggestion = stripped.to_string(); | ||||||
match validate_name(&suggestion) { | ||||||
Ok(_) => Some(suggestion), | ||||||
Err(_) => None, | ||||||
} | ||||||
} | ||||||
_ => None, | ||||||
}, | ||||||
InvalidProjectNameReason::ErlangReservedWord => Some(format!("{}_app", invalid_name)), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reserved words of Erlang we suggest the project name with an Does that make sense? |
||||||
InvalidProjectNameReason::ErlangStandardLibraryModule => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For standard library module names of Erlang we suggest the project name with an Does that make sense? |
||||||
Some(format!("{}_app", invalid_name)) | ||||||
} | ||||||
InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reserved words of Gleam we suggest the project name with an Does that make sense? |
||||||
InvalidProjectNameReason::GleamReservedModule => Some(format!("{}_app", invalid_name)), | ||||||
InvalidProjectNameReason::FormatNotLowercase => Some(invalid_name.to_lowercase()), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case the issue is related to casing only, we suggest the project name in lowercase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one could be invalid still! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Line 348 in 91445da
} else if regex::Regex::new("^[a-zA-Z][a-zA-Z0-9_]*$")
.expect("failed regex to match valid but non-lowercase name format")
.is_match(name)
{
Err(Error::InvalidProjectName {
name: name.to_string(),
reason: InvalidProjectNameReason::FormatNotLowercase,
})
} else { Thats why I added
There is also a test for it: gleam/compiler-cli/src/new/tests.rs Line 409 in 91445da
assert_eq!(
crate::new::suggest_valid_name(
"Project_Name",
&crate::new::InvalidProjectNameReason::FormatNotLowercase
),
Some("project_name".to_string())
); There were no tests for this at all, so I added some for each Or did I miss something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be addressed, in case you missed my comment! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes – have you read my answer to your comment, @lpil? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see In which cases this might be invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the detailed explanation, I did not get it at first, my bad |
||||||
InvalidProjectNameReason::Format => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case the project name is valid after:
we suggest that name. Otherwise we don't and the (The logic is not smart enough to keep potentially preexisting, consecutive underscores of the initial name.) Stripping any non letter characters from the beginning of the project name is not implemented yet. Shall we? |
||||||
let suggestion = regex::Regex::new(r"[^a-z0-9]") | ||||||
.expect("failed regex to match any non-lowercase and non-alphanumeric characters") | ||||||
.replace_all(&invalid_name.to_lowercase(), "_") | ||||||
.to_string(); | ||||||
|
||||||
let suggestion = regex::Regex::new(r"_+") | ||||||
.expect("failed regex to match consecutive underscores") | ||||||
.replace_all(&suggestion, "_") | ||||||
.to_string(); | ||||||
|
||||||
match validate_name(&suggestion) { | ||||||
Ok(_) => Some(suggestion), | ||||||
Err(_) => None, | ||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
fn get_valid_project_name(name: Option<String>, project_root: &str) -> Result<String, Error> { | ||||||
let initial_name = match name { | ||||||
Some(name) => name, | ||||||
None => get_foldername(project_root)?, | ||||||
} | ||||||
.trim() | ||||||
.to_string(); | ||||||
|
||||||
let invalid_reason = match validate_name(&initial_name) { | ||||||
Ok(_) => return Ok(initial_name), | ||||||
Err(Error::InvalidProjectName { reason, .. }) => reason, | ||||||
Err(error) => return Err(error), | ||||||
}; | ||||||
|
||||||
let suggested_name = match suggest_valid_name(&initial_name, &invalid_reason) { | ||||||
Some(suggested_name) => suggested_name, | ||||||
None => { | ||||||
return Err(Error::InvalidProjectName { | ||||||
name: initial_name, | ||||||
reason: invalid_reason, | ||||||
}) | ||||||
} | ||||||
}; | ||||||
let prompt_for_suggested_name = error::format_invalid_project_name_error( | ||||||
&initial_name, | ||||||
&invalid_reason, | ||||||
&Some(suggested_name.clone()), | ||||||
); | ||||||
match crate::cli::confirm(&prompt_for_suggested_name)? { | ||||||
true => Ok(suggested_name), | ||||||
false => Err(Error::InvalidProjectName { | ||||||
name: initial_name, | ||||||
reason: invalid_reason, | ||||||
}), | ||||||
} | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -524,13 +524,60 @@ impl From<capnp::NotInSchema> for Error { | |
#[derive(Debug, PartialEq, Eq, Clone, Copy)] | ||
pub enum InvalidProjectNameReason { | ||
Format, | ||
FormatNotLowercase, | ||
GleamPrefix, | ||
ErlangReservedWord, | ||
ErlangStandardLibraryModule, | ||
GleamReservedWord, | ||
GleamReservedModule, | ||
} | ||
|
||
pub fn format_invalid_project_name_error( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extracted this so that it can be reused in |
||
name: &str, | ||
reason: &InvalidProjectNameReason, | ||
with_suggestion: &Option<String>, | ||
) -> String { | ||
let reason_message = match reason { | ||
InvalidProjectNameReason::ErlangReservedWord => "is a reserved word in Erlang.", | ||
InvalidProjectNameReason::ErlangStandardLibraryModule => { | ||
"is a standard library module in Erlang." | ||
} | ||
InvalidProjectNameReason::GleamReservedWord => "is a reserved word in Gleam.", | ||
InvalidProjectNameReason::GleamReservedModule => "is a reserved module name in Gleam.", | ||
InvalidProjectNameReason::FormatNotLowercase => { | ||
"does not have the correct format. Project names \ | ||
may only contain lowercase letters." | ||
} | ||
InvalidProjectNameReason::Format => { | ||
"does not have the correct format. Project names \ | ||
must start with a lowercase letter and may only contain lowercase letters, \ | ||
numbers and underscores." | ||
} | ||
InvalidProjectNameReason::GleamPrefix => { | ||
"has the reserved prefix `gleam_`. \ | ||
This prefix is intended for official Gleam packages only." | ||
} | ||
}; | ||
|
||
match with_suggestion { | ||
Some(suggested_name) => wrap_format!( | ||
"We were not able to create your project as `{}` {} | ||
|
||
Would you like to name your project '{}' instead?", | ||
name, | ||
reason_message, | ||
suggested_name | ||
), | ||
None => wrap_format!( | ||
"We were not able to create your project as `{}` {} | ||
|
||
Please try again with a different project name.", | ||
name, | ||
reason_message | ||
), | ||
} | ||
} | ||
|
||
#[derive(Debug, PartialEq, Eq, Clone, Copy)] | ||
pub enum StandardIoAction { | ||
Read, | ||
|
@@ -822,29 +869,7 @@ of the Gleam dependency modules." | |
} | ||
|
||
Error::InvalidProjectName { name, reason } => { | ||
let text = wrap_format!( | ||
"We were not able to create your project as `{}` {} | ||
|
||
Please try again with a different project name.", | ||
name, | ||
match reason { | ||
InvalidProjectNameReason::ErlangReservedWord => | ||
"is a reserved word in Erlang.", | ||
InvalidProjectNameReason::ErlangStandardLibraryModule => | ||
"is a standard library module in Erlang.", | ||
InvalidProjectNameReason::GleamReservedWord => | ||
"is a reserved word in Gleam.", | ||
InvalidProjectNameReason::GleamReservedModule => | ||
"is a reserved module name in Gleam.", | ||
InvalidProjectNameReason::Format => | ||
"does not have the correct format. Project names \ | ||
must start with a lowercase letter and may only contain lowercase letters, \ | ||
numbers and underscores.", | ||
InvalidProjectNameReason::GleamPrefix => | ||
"has the reserved prefix `gleam_`. \ | ||
This prefix is intended for official Gleam packages only.", | ||
} | ||
); | ||
let text = format_invalid_project_name_error(name, reason, &None); | ||
|
||
vec![Diagnostic { | ||
title: "Invalid project name".into(), | ||
|
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.
In case the project name is valid after stripping the
gleam_
prefix, we suggest that name.Otherwise we don't and the
Error
module does its thing.