-
Notifications
You must be signed in to change notification settings - Fork 321
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
Allow users to give a project other than Upper_Snake_Case name #7397
Allow users to give a project other than Upper_Snake_Case name #7397
Conversation
One question is, whether the IDE, when started, should show the module name or the display name of the project |
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.
the changes to JS code are minimal, so there shouldn't be any problems
app/ide-desktop/lib/dashboard/src/authentication/src/dashboard/validation.ts
Outdated
Show resolved
Hide resolved
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.
I've checked the rust part. Looks ok, but see my comment about displaying project name.
@@ -111,8 +111,7 @@ pub struct ProjectName(String); | |||
impl ProjectName { | |||
/// Create new ProjectName without any validation. | |||
/// | |||
/// The caller is responsible for making sure that provided string is a valid project name | |||
/// (e.g. not empty and starts with a capital letter). | |||
/// The caller is responsible for making sure that provided string is a valid project name. |
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.
Are there invalid project names yet? If not, this ProjectName
struct could be just replaced with string.
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.
Technically there are some restrictions. It cannot be empty i.e. should contain at least one non-whitespace character. I would keep this newtype just to not confuse it with the module name or other strings.
@@ -422,13 +422,13 @@ impl Project { | |||
let action = MissingComponentAction::Install; | |||
let opened = project_manager.open_project(&id, &action).await?; | |||
let namespace = opened.project_namespace; | |||
let name = opened.project_name; | |||
let module = opened.project_module; |
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.
We need to keep also the "real name" of the project, because now the "module" name will be displayed in GUI (in the left-top corner).
This should be fairy easy. We can also do that as a follow-up of this PR, depending on priorities.
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.
I tried to update the IDE today, but it very quickly unfolded in a quite large change. I created an issue to fix it later #7409
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.
Few questions. For example I am not sure why should we use moduleName
terminology?
...project-manager/src/main/scala/org/enso/projectmanager/service/MonadicProjectValidator.scala
Show resolved
Hide resolved
object ModuleNameValidator { | ||
|
||
/** Base trait for validations failures. */ | ||
sealed trait ValidationFailure |
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.
Why do we have two ValidationFailure
traits? One here and another in ProjectNameValidator
?
...manager/src/main/scala/org/enso/projectmanager/service/validation/ProjectNameValidator.scala
Show resolved
Hide resolved
binHost <- binAddr.downField("host").as[String] | ||
binPort <- binAddr.downField("port").as[Int] | ||
projectName <- result.downField("projectName").as[String] | ||
projectModule <- result.downField("projectModule").as[String] |
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.
Is this changing content of package.yaml
file? If so, then update /docs/distribution/packaging.md
as that seems to contain the specification of the content of that file.
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.
No, it just parses the JSON-RPC response. But I should update the packaging.md
to include optional module
field 👍
We decided to use |
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.
Minus Jaroslav's comments and the s/moduleName/normalizedName
, the rest looks OK
normalizeName("\uD83D\uDE80") shouldEqual "Project" | ||
normalizeName("\uD83D\uDE80_\uD83D\uDE0A") shouldEqual "Project" | ||
normalizeName("\uD83D\uDE80_\uD83D\uDE0A__") shouldEqual "Project" | ||
normalizeName("_\uD83D\uDE80_\uD83D\uDE0A_") shouldEqual "Project" |
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.
so a project name like $$$
would also end up being named Project
?
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.
Yes, if the name does not have any legal (alphanumeric) characters, it defaults to Project
Followup to #7397. When started this refactoring I only changed the config types and forgot to change the API. Changelog: - refactor: rename `projectModule` to `projectNormalizedName` in the API # Important Notes https://github.com/enso-org/enso/assets/357683/a840dc90-15f8-4b6d-bbfb-571c53cd2f7d
Pull Request Description
close #6356
Allow arbitrary names for user projects.
Important Notes
enso-rename-project-arbitrary-name.mp4
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.