-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ensure that an appropriate name is assigned to the generated template #1119
Conversation
@@ -217,6 +217,8 @@ function copyCommon(appDir: string, sharedConfigDir: string) { | |||
); | |||
|
|||
// Rewrite package.json | |||
packageObjs.name = appDir.replace(/.*\//, ""); |
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 think it might be a good to let users determine the package name from the directory they specify, considering the common usage.
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.
BTW, I would like to implement it someday. but currently, there is no established ci testing flow to verify if the generated template is functioning correctly.
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.
BTW, I would like to implement it someday. but currently, there is no established ci testing flow to verify if the generated template is functioning correctly.
Yes, I had the same thought when I was cleaning the script. 😅
I think this we should consider this a high priority task
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 think it might be a good to let users determine the package name from the directory they specify, considering the common usage.
In the future, it may be better to add an additional prompt in the script to ask the user to specify the package name. There may be cases where the directory name and the package name are not the same.
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 created an issue #1125 for now.
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.
LGTM for now, but as I've written in my comment, I think it would be better to add an additional prompt for this later on.
@@ -217,6 +217,8 @@ function copyCommon(appDir: string, sharedConfigDir: string) { | |||
); | |||
|
|||
// Rewrite package.json | |||
packageObjs.name = appDir.replace(/.*\//, ""); |
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.
BTW, I would like to implement it someday. but currently, there is no established ci testing flow to verify if the generated template is functioning correctly.
Yes, I had the same thought when I was cleaning the script. 😅
I think this we should consider this a high priority task
@@ -217,6 +217,8 @@ function copyCommon(appDir: string, sharedConfigDir: string) { | |||
); | |||
|
|||
// Rewrite package.json | |||
packageObjs.name = appDir.replace(/.*\//, ""); |
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 think it might be a good to let users determine the package name from the directory they specify, considering the common usage.
In the future, it may be better to add an additional prompt in the script to ask the user to specify the package name. There may be cases where the directory name and the package name are not the same.
@ptrkdan Thank you for the review! I've also created an issue, so I'll proceed with the merge for now. |
What I did
Fixed #1118
How to test