LS: warn when a Gleam file path is not a valid module name #5015
Conversation
lpil
left a comment
lpil
left a comment
There was a problem hiding this comment.
Lovely! Thank you very much. I've added some feedback inline, and we'll also need tests for all the new functionality.
lpil
changed the title
|
Hello! If this is ready for review remember to mark is as ready so we can have a look :) |
ready |
giacomocavalieri
left a comment
giacomocavalieri
left a comment
There was a problem hiding this comment.
There's some changes I'm not sure why they were introduced
| ]; | ||
| assert_eq!( | ||
| parse_and_order(functions.as_slice(), [].as_slice()).unwrap(), | ||
| parse_and_order(functions.as_slice(), [].as_slice()).expect("Expected parse_and_order to succeed"), |
There was a problem hiding this comment.
Undo these unrelated changes please
| hint: None, | ||
| }; | ||
| feedback.append_diagnostic(path.to_path_buf(), diagnostic); | ||
| Some(feedback) |
There was a problem hiding this comment.
Return a diagnostic from this function, not the entire feedback container, otherwise this function would be responsible for checking for the mistake and also for the higher level concern of how they are stored.
| s = "module".into(); | ||
| } | ||
| if !s.chars().next().map(|c| c.is_ascii_lowercase()).unwrap_or(false) { | ||
| s = format!("m{}", s); |
| let suggestion = module_name | ||
| .split('/') | ||
| .map(|seg| { | ||
| let mut s = to_snake_case(&ecow::EcoString::from(seg)).to_string(); |
There was a problem hiding this comment.
Use ecostring rather than string please
| s | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join("/"); |
There was a problem hiding this comment.
This can suggest invalid module names, such as gleam.
|
|
||
| let mut feedback = Feedback::default(); | ||
| // Highlight the first line to ensure the diagnostic is visible in editors | ||
| let first_line_end = src.find('\n').unwrap_or(src.len()); |
There was a problem hiding this comment.
How does this look in various editors? Could we get some screenshots 🙏
| let first_line_end = src.find('\n').unwrap_or(src.len()); | ||
| let span_end: u32 = first_line_end.max(1).try_into().unwrap_or(1); | ||
| let diagnostic = Diagnostic { | ||
| title: format!("{module_name} is not a valid name for a Gleam module"), |
There was a problem hiding this comment.
| title: format!("{module_name} is not a valid name for a Gleam module"), | |
| title: format!("{module_name} is not a valid module name"), |
| let span_end: u32 = first_line_end.max(1).try_into().unwrap_or(1); | ||
| let diagnostic = Diagnostic { | ||
| title: format!("{module_name} is not a valid name for a Gleam module"), | ||
| text: format!("You should switch to {suggestion} instead."), |
There was a problem hiding this comment.
Include what the rules are for module names, and don't use the word "should", especially in times like this when we don't know what the best option is.
| _ => return false, | ||
| } | ||
| iter.all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_') | ||
| } |
There was a problem hiding this comment.
Can't we use the existing code for validating module names?
3 participants
fix issue #3972