Suggest valid project names on InvalidProjectName #3913

diemogebhardt · 2024-11-29T19:12:35Z

This PR is regarding issue #1728.

Depending on the InvalidProjectNameReason we might suggest a valid project name.

In case of the newly added InvalidProjectNameReason::FormatNotLowercase it looks like so:

We were not able to create your project as `Project_Name` does not have the
correct format. Project names may only contain lowercase letters.

Would you like to name your project 'project_name' instead? [y/n]:

Lets take this as a basis to discuss what valid project names we might want to suggest for which InvalidProjectNameReason.

diemogebhardt · 2024-11-29T19:46:29Z

Is there any guideline when to use at the top of the file or when to use the fully qualified names? I couldn't recognize a pattern.

GearsDatapacks · 2024-11-29T20:47:53Z

I use pretty much everything. I think it's up to you really

lpil

Very nice! I've left some small things inline.

When you are ready for a review please un-draft this PR. Thank you!

lpil · 2024-12-02T16:59:41Z

compiler-cli/src/new.rs

+                }),
+            }
+        }
+        Err(error) => Err(error),


This function is quite hard to read! Could you rewrite it to use early returns rather than nested flow control please 🙏

lpil · 2024-12-02T17:00:26Z

compiler-cli/src/new.rs

+            Some(format!("{}_app", invalid_name))
+        }
+        InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::GleamReservedModule => None,


How come this one doesn't use this _app suffix?

Ah, makes sense – it does now! :)

lpil · 2024-12-05T14:28:14Z

compiler-cli/src/new.rs

+        }
+        InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::GleamReservedModule => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::FormatNotLowercase => Some(invalid_name.to_lowercase()),


This one could be invalid still!

In fn validate_name we make sure to return FormatNotLowercase only, if lowercase is the only issue:

gleam/compiler-cli/src/new.rs

Line 348 in 91445da

} else if regex::Regex::new("^[a-zA-Z][a-zA-Z0-9_]*$")

} 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 FormatNotLowercase, to simplify the logic in fn suggest_valid_name.

In fn validate_name we focus on validation and classification only.

In fn suggest_valid_name we focus on suggesting only.

There is also a test for it:

gleam/compiler-cli/src/new/tests.rs

Line 409 in 91445da

"Project_Name",

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 InvalidProjectNameReason.

Or did I miss something?

This needs to be addressed, in case you missed my comment!

Yes – have you read my answer to your comment, @lpil?

I don't see In which cases this might be invalid.

Thank you for the detailed explanation, I did not get it at first, my bad

lpil · 2024-12-17T14:31:55Z

Looking good but! There's some comments of mine above that still need to be addressed!

diemogebhardt · 2024-12-18T02:29:44Z

Looking good but! There's some comments of mine above that still need to be addressed!

I give up as you either don't seem to see or simply don't read my replies. They have all been addressed, @lpil:

Feel free to close this unless you plan to read and reply to my replies to your comments from above. How frustrating.

lpil · 2024-12-20T14:47:57Z

Any comment with the "pending" thingy on it is a draft that you have not published yet, I cannot see them!

diemogebhardt · 2024-11-29T19:27:09Z

compiler-cli/src/new.rs

+
+fn suggest_valid_name(invalid_name: &str, reason: &InvalidProjectNameReason) -> Option<String> {
+    match reason {
+        InvalidProjectNameReason::GleamPrefix => match invalid_name.strip_prefix("gleam_") {


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.

diemogebhardt · 2024-11-29T19:28:21Z

compiler-cli/src/new.rs

+            }
+            _ => None,
+        },
+        InvalidProjectNameReason::ErlangReservedWord => Some(format!("{}_app", invalid_name)),


For reserved words of Erlang we suggest the project name with an _app suffix.

Does that make sense?

diemogebhardt · 2024-11-29T19:29:18Z

compiler-cli/src/new.rs

+            _ => None,
+        },
+        InvalidProjectNameReason::ErlangReservedWord => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::ErlangStandardLibraryModule => {


For standard library module names of Erlang we suggest the project name with an _app suffix.

Does that make sense?

diemogebhardt · 2024-11-29T19:29:44Z

compiler-cli/src/new.rs

+        InvalidProjectNameReason::ErlangStandardLibraryModule => {
+            Some(format!("{}_app", invalid_name))
+        }
+        InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)),


For reserved words of Gleam we suggest the project name with an _app suffix.

Does that make sense?

diemogebhardt · 2024-11-29T19:30:34Z

compiler-cli/src/new.rs

+            Some(format!("{}_app", invalid_name))
+        }
+        InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::GleamReservedModule => None,


There is nothing to suggest in case the project name is just gleam.

So the Error module does its thing.

For the reserved module Gleam we suggest the project name with an _app suffix.

Does that make sense?

diemogebhardt · 2024-12-04T14:05:04Z

compiler-cli/src/new.rs

+            Some(format!("{}_app", invalid_name))
+        }
+        InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::GleamReservedModule => None,


Ah, makes sense – it does now! :)

diemogebhardt · 2024-12-04T16:24:37Z

compiler-cli/src/new.rs

+                }),
+            }
+        }
+        Err(error) => Err(error),


diemogebhardt · 2024-12-06T01:06:08Z

compiler-cli/src/new.rs

+        }
+        InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::GleamReservedModule => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::FormatNotLowercase => Some(invalid_name.to_lowercase()),


In fn validate_name we make sure to return FormatNotLowercase only, if lowercase is the only issue:

gleam/compiler-cli/src/new.rs

Line 348 in 91445da

} else if regex::Regex::new("^[a-zA-Z][a-zA-Z0-9_]*$")

} 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 FormatNotLowercase, to simplify the logic in fn suggest_valid_name.

In fn validate_name we focus on validation and classification only.

In fn suggest_valid_name we focus on suggesting only.

There is also a test for it:

gleam/compiler-cli/src/new/tests.rs

Line 409 in 91445da

"Project_Name",

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 InvalidProjectNameReason.

Or did I miss something?

diemogebhardt · 2024-12-10T21:04:58Z

compiler-cli/src/new.rs

+        }
+        InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::GleamReservedModule => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::FormatNotLowercase => Some(invalid_name.to_lowercase()),


Yes – have you read my answer to your comment, @lpil?

diemogebhardt · 2024-12-10T21:07:32Z

compiler-cli/src/new.rs

+        }
+        InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::GleamReservedModule => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::FormatNotLowercase => Some(invalid_name.to_lowercase()),


I don't see In which cases this might be invalid.

diemogebhardt · 2024-12-23T23:46:45Z

Any comment with the "pending" thingy on it is a draft that you have not published yet, I cannot see them!

Ok, this explains everything... 🤦 I wasn't aware of this and got quite frustrated that you seemed to ignore all my comments across all of my PRs. My mistake – sorry, @lpil!

diemogebhardt · 2024-12-23T23:51:03Z

Looking good but! There's some comments of mine above that still need to be addressed!

Can you have a look at this #3913 (comment) please? This should explain why the handling case of InvalidProjectNameReason::FormatNotLowercase cannot be invalid.

lpil

Thank you!

lpil · 2025-01-09T13:52:35Z

compiler-cli/src/new.rs

+        }
+        InvalidProjectNameReason::GleamReservedWord => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::GleamReservedModule => Some(format!("{}_app", invalid_name)),
+        InvalidProjectNameReason::FormatNotLowercase => Some(invalid_name.to_lowercase()),


Thank you for the detailed explanation, I did not get it at first, my bad

… InvalidProjectNameReason

…ectNameReason

diemogebhardt force-pushed the suggest-valid-project-names branch from e22a663 to acc8f59 Compare November 29, 2024 19:23

diemogebhardt mentioned this pull request Nov 29, 2024

Restricted Project Naming #1728

Open

lpil reviewed Dec 2, 2024

View reviewed changes

lpil marked this pull request as draft December 2, 2024 17:01

diemogebhardt force-pushed the suggest-valid-project-names branch 2 times, most recently from 88549dc to de48a84 Compare December 4, 2024 14:06

diemogebhardt marked this pull request as ready for review December 4, 2024 16:24

lpil reviewed Dec 5, 2024

View reviewed changes

lpil marked this pull request as draft December 9, 2024 16:18

diemogebhardt marked this pull request as ready for review December 10, 2024 21:05

diemogebhardt force-pushed the suggest-valid-project-names branch from a8f8f29 to 7d5edec Compare December 10, 2024 21:40

lpil marked this pull request as draft December 17, 2024 14:32

diemogebhardt commented Dec 23, 2024

View reviewed changes

diemogebhardt marked this pull request as ready for review December 23, 2024 23:52

diemogebhardt force-pushed the suggest-valid-project-names branch from 7d5edec to 9eece7f Compare December 24, 2024 00:19

lpil approved these changes Jan 9, 2025

View reviewed changes

diemogebhardt added 5 commits January 9, 2025 13:53

Add FormatNotLowercase as another InvalidProjectNameReason

39f2e55

Add project name validation tests re Format and FormatNotLowercase of…

9f83045

… InvalidProjectNameReason

Extract error message generation of InvalidProjectName

014a844

Add mechanism to suggest valid project names depending on InvalidProj…

2e9bfd4

…ectNameReason

Add project suggestion tests depending on InvalidProjectNameReason

677f0b3

diemogebhardt added 3 commits January 9, 2025 13:53

Add valid project name suggestion for GleamReservedModule

cb5ef3f

Refactor project name validation to reduce nesting

9858254

Add credits to CHANGELOG

c999e7b

lpil force-pushed the suggest-valid-project-names branch from 9eece7f to c999e7b Compare January 9, 2025 13:54

lpil merged commit 097677e into gleam-lang:main Jan 9, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest valid project names on InvalidProjectName #3913

Suggest valid project names on InvalidProjectName #3913

diemogebhardt commented Nov 29, 2024

diemogebhardt commented Nov 29, 2024

GearsDatapacks commented Nov 29, 2024

lpil left a comment •

edited

Loading

lpil Dec 2, 2024

diemogebhardt Dec 4, 2024

lpil Dec 2, 2024

diemogebhardt Dec 4, 2024

lpil Dec 5, 2024

diemogebhardt Dec 6, 2024

lpil Dec 9, 2024

diemogebhardt Dec 10, 2024

diemogebhardt Dec 10, 2024

lpil Jan 9, 2025

lpil commented Dec 17, 2024

diemogebhardt commented Dec 18, 2024

lpil commented Dec 20, 2024 •

edited

Loading

diemogebhardt Nov 29, 2024

diemogebhardt Nov 29, 2024

diemogebhardt Nov 29, 2024

diemogebhardt Nov 29, 2024

diemogebhardt Nov 29, 2024

diemogebhardt Dec 4, 2024

diemogebhardt Dec 4, 2024

diemogebhardt Dec 4, 2024

diemogebhardt Dec 6, 2024

diemogebhardt Dec 10, 2024

diemogebhardt Dec 10, 2024

diemogebhardt commented Dec 23, 2024 •

edited

Loading

diemogebhardt commented Dec 23, 2024

lpil left a comment

lpil Jan 9, 2025

Suggest valid project names on InvalidProjectName #3913

Suggest valid project names on InvalidProjectName #3913

Conversation

diemogebhardt commented Nov 29, 2024

diemogebhardt commented Nov 29, 2024

GearsDatapacks commented Nov 29, 2024

lpil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil commented Dec 17, 2024

diemogebhardt commented Dec 18, 2024

lpil commented Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diemogebhardt commented Dec 23, 2024 • edited Loading

diemogebhardt commented Dec 23, 2024

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil left a comment •

edited

Loading

lpil commented Dec 20, 2024 •

edited

Loading

diemogebhardt commented Dec 23, 2024 •

edited

Loading