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

Validate name when instantiating template #740

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

itowlson
Copy link
Contributor

@itowlson itowlson commented Sep 7, 2022

Fixes #739.

Output:

$ spin new http-rust 123test
error: Invalid value "123test" for '<NAME>': Name must start with a letter

Notes for reviewers:

  1. This validates in the CLI, rather than in the template crate itself. Is this reasonable or should we push the validation into that shared core?

  2. This makes no consideration for language-specific naming rules. E.g. if a language used the name only for directory and component names, this restriction would be unnecessary. Is this okay or should we move the validation somewhere that understands per-language or per-template constraints?

  3. This forbids problematic names. An alternative approach would be to transform them into legal ones, which could be done via Liquid filters in the templates (again allowing more language-specific understanding) - e.g. transform 34 to spinapp34. What do we prefer?

Signed-off-by: itowlson ivan.towlson@fermyon.com

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Copy link
Contributor

@fibonacci1729 fibonacci1729 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 to me.

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

I think it makes sense to catch it with clap for the time being. If other languages allow non-letter first names and an issue comes up around that, then we could switch, but prioritizing early and simple checking seems appropriate. LGTM!

@itowlson itowlson merged commit 16455ab into fermyon:main Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using a digit as the starting character of a rust component fails to build
3 participants