Skip to content
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

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

Merged
merged 29 commits into from
Mar 21, 2024

Conversation

Halimao
Copy link
Contributor

@Halimao Halimao commented Jan 11, 2024

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.

@kayagokalp kayagokalp added enhancement New feature or request forc labels Jan 12, 2024
@kayagokalp kayagokalp requested a review from a team January 12, 2024 13:21
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Halimao
Copy link
Contributor Author

Halimao commented Jan 15, 2024

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

@sdankel
Copy link
Member

sdankel commented Mar 2, 2024

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-_]+)|)$

@Halimao
Copy link
Contributor Author

Halimao commented Mar 3, 2024

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?

@sdankel
Copy link
Member

sdankel commented Mar 3, 2024

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

@Halimao
Copy link
Contributor Author

Halimao commented Mar 4, 2024

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?

@sdankel
Copy link
Member

sdankel commented Mar 5, 2024

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.

forc-util/src/restricted.rs Outdated Show resolved Hide resolved
@Halimao
Copy link
Contributor Author

Halimao commented Mar 6, 2024

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~

forc-pkg/src/manifest/mod.rs Outdated Show resolved Hide resolved
forc-util/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@sdankel sdankel enabled auto-merge (squash) March 13, 2024 23:18
@Halimao
Copy link
Contributor Author

Halimao commented Mar 16, 2024

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

@sdankel sdankel merged commit 4d23798 into FuelLabs:master Mar 21, 2024
35 checks passed
@Halimao Halimao deleted the fix/forc-init branch March 21, 2024 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request forc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants