sway icon indicating copy to clipboard operation
sway copied to clipboard

refactor: `forc init` generate unexpected project_name when project dir contains dot

Open Halimao opened this issue 1 year ago • 6 comments

Description

Refer #5434

Found this issue when I tried to run forc init --script under a directory(/tmp/path_with_._dot) that contains a dot ".". The value of project.name in the generated file Forc.toml is truncated by the dot "." as path_with_.

Since project_dir must be a directory(has checked before), we can use file_name instead of file_stem to get the last-level dir name. Also, I think it would be better to replace "." with "_", otherwise a project_name that contains . would fail with validation validate_name(&project_name, "project name")?;

Screenshots

Before:

image

After:

image

Checklist

  • [ ] I have linked to any relevant issues.
  • [ ] I have commented my code, particularly in hard-to-understand areas.
  • [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • [ ] I have added tests that prove my fix is effective or that my feature works.
  • [ ] I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • [ ] I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • [ ] I have requested a review from the relevant team or maintainers.

Halimao avatar Jan 11 '24 10:01 Halimao

Thanks for the PR! I will test this locally but it seems like this will allow project with . in the name, which is not something we should be doing imo. Blocking for now so that I can test

The . will be replaced with _. This pr just changed the value of project_name by using path_with___dot instead of path_with_. See the screenshot below:

image

Halimao avatar Jan 15 '24 01:01 Halimao

I think we should disallow project names with dots or any special character other than _, similar to cargo. Numbers should be allowed but not as a first digit.

Here's a regex we can use: ^([a-zA-Z]([a-zA-Z0-9-_]+)|)$

sdankel avatar Mar 02 '24 21:03 sdankel

I think we should disallow project names with dots or any special character other than _, similar to cargo. Numbers should be allowed but not as a first digit.

Here's a regex we can use: ^([a-zA-Z]([a-zA-Z0-9-_]+)|)$

If I understood correctly, the suggested changes are to revert the logic of getting project_name and add a new regex ^([a-zA-Z]([a-zA-Z0-9-_]+)|)$ checker into the function validate_name?

Halimao avatar Mar 03 '24 05:03 Halimao

I think we should disallow project names with dots or any special character other than _, similar to cargo. Numbers should be allowed but not as a first digit. Here's a regex we can use: ^([a-zA-Z]([a-zA-Z0-9-_]+)|)$

If I understood correctly, the suggested changes are to revert the logic of getting project_name and add a new regex ^([a-zA-Z]([a-zA-Z0-9-_]+)|)$ checker into the function validate_name?

Yes, I think that's the best path forward. If it doesn't match the regex, we can show an error like "The project name can be a combination of letters, numbers and underscores, and must start with a letter."

sdankel avatar Mar 03 '24 21:03 sdankel

Yes, I think that's the best path forward. If it doesn't match the regex, we can show an error like "The project name can be a combination of letters, numbers, and underscores, and must start with a letter."

image

  1. The function validate_name is not only used for project name but also for organization name and package name
  2. The param use_case is not an enum, there may exist different values for the similar case project name, and it's hard to judge whether the case is project name or not

@sdankel For the reason above, I think we should add a new function validate_project_name like this:

pub fn validate_project_name(name: &str) -> Result<()> {
    let re = Regex::new(r"^([a-zA-Z]([a-zA-Z0-9-_]+)|)$").unwrap();
    if !re.is_match(name) {
        bail!(
            "the project name `{name}` cannot be used as a project name.\n\
        project name can be a combination of letters, numbers, hyphen, and underscores, \
        and must start with a letter."
        );
    }
    validate_name(name, "project name")
}

BTW, is the '-' hyphen valid in the project name?

Halimao avatar Mar 04 '24 03:03 Halimao

BTW, is the '-' hyphen valid in the project name?

Yes, good catch! - should be allowed.

The function validate_name is not only used for project name but also for organization name and package name

I don't think there is a difference between package name and project name. I agree that organization name should not have this validation.

sdankel avatar Mar 05 '24 19:03 sdankel

BTW, is the '-' hyphen valid in the project name?

Yes, good catch! - should be allowed.

The function validate_name is not only used for project name but also for organization name and package name

I don't think there is a difference between package name and project name. I agree that organization name should not have this validation.

Suggestion changes done~

Halimao avatar Mar 06 '24 10:03 Halimao

Can we get this merged now? Is there anything else that needs to be added?

Halimao avatar Mar 16 '24 08:03 Halimao