-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(cli): change to lowercase the project name type by the user #6540
fix(cli): change to lowercase the project name type by the user #6540
Conversation
@javiersantossanchez thank you for the pull request. Please add a test to verify the new behavior. Could you also please fix the commit message to follow our Commit message guidelines? Travis CI is complaining about too long lines: https://travis-ci.com/github/strongloop/loopback-next/jobs/397830225#L232
If you are not familiar with history rewriting, you want to run I'll leave it up to code owners (@emonddr @agnes512) to review your proposed changes and merge the pull request once is ready. |
for the project generator, when the user use a project name with uppercase letters, the project can not be create because npm packages has a validation to no allow create packages with uppercase letters, the prompt was update to add a function to transform the values typed by the user to lowercase version and use this value as the project name. Signed-off-by: javier santos <javier.david.santos@gmail.com>
983a82a
to
e707abd
Compare
@bajtos, I update the commit message, and now there is no warning on the validation. Thank you for your comment about how to update the commit message. I tried to create some unit tests, But I do not find a way to implement a unit test to evaluate functions on prompts using yeoman-generators. If you can help me to understand how I can test my changes. I would be happy to add some tests. |
Thank you for the PR! Could you add a test to verify the changes? I think this file is a good place for it. |
Hi @agnes512 agnes512. Yes, I am trying to test my changes on the prompt configuration. Actually what I did was add a filter and transformer function (as is defined in the api https://github.com/SBoudrias/Inquirer.js). My approach was to add those functions to change any value enter by the used to uppercase value, the output of the prompt is already a lowercase value. Actually I try to add this test. but the transformer function is no called. the idea is verify that when the user type "fooBar" the output should be "foobar"
I not sure if there is something more I can do, to complete the test. I feel that a test is missing to verify that the prompt resturns the right value when the user key uppercase letters. |
Oh, I forgot again how difficult it is to test Yeoman prompts 😠 (Ref #844). I am fine to land these changes with no automated tests then. |
@javiersantossanchez I ran your feature branch and it seems like your solution is not fixing the problem. Does it work as intended on your machine?
|
8223d70
to
570eaea
Compare
Hi @bajtos , Yes I tested :-( . I think that I test my initial approach (use a regular toLower() function) then I change to use the lowerCase function on utils.js file, maybe I do not clean my test and I just run my previous approach. Sorry about it. Well, you are right, my commit was not working, I found a problem with the import of lowerCase function. I fix it on the utils.js, I found no one else is using this function. And sorry again for my mistake in testing my change. |
the lowercase function is no exported on change-case package as the official documentation https://github.com/blakeembrey/change-case shows Signed-off-by: javier santos <javier.david.santos@gmail.com>
60901d2
to
6d3e764
Compare
method created for tes on development Signed-off-by: javier santos <javier.david.santos@gmail.com>
6d3e764
to
ae40a85
Compare
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.
Tested on my machine, and it works 👍 I have one suggestion in terms of UX. Would it be better to have a hint that the project name will be transformed to lowercase? In case some users are not 100% familiar with npm/yarn. Feel free to ignore though 😄
+1. When trying out this new CLI behavior, I have to confess that I didn't look through the code and expecting I'll receive an warning (as mentioned in #6540 (comment)). Personally I'd prefer to have CLI giving warning about invalid name or case. |
This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity. |
for the project generator, when the user uses a project name with uppercase letters, the project can not be created because npm packages have validation to no allow create packages with uppercase letters
The prompt was updated to add a function to transform the values typed by the user to the lowercase version and use this value as the project name
Fixes #1663
#1663
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updatedSigned-off-by: javier santos javier.david.santos@gmail.com