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

fix(cli): fix app default project name. untildify the project path #3252

Merged
merged 2 commits into from
Jul 15, 2019

Conversation

agnes512
Copy link
Contributor

@agnes512 agnes512 commented Jun 26, 2019

Description

In the issue 2092, the default project name could be generated incorrectly if the cwd folder name has hyphens, fixed. The input app name has no such issue.

The user also suggested that lb4 should allow the project to be generated in the CWD. However, that could be messy and confusing. The other issue is that lb4 allows user to use any project path as they want. This could be problematic:

(CWD: User/lb4-app)
? Project name: pathFromRoot
? Project root directory:  ~/try/pathFromRoot
...
Application pathFromRoot was created in ~/try/pathFromRoot

lb4-app$ ls
~
lb4-app$ cd ~
[goes to the root folder]

The change here is to untildify the input path to make sure that it always uses absolute path.

resolves #2092

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@raymondfeng
Copy link
Contributor

The user also suggested that lb4 should allow the project to be generated in the CWD. However, that could be messy and confusing.

I agree. Can the user type . for his case?

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

Nice fix 👍. Can we add a test for the changes?

packages/cli/lib/utils.js Outdated Show resolved Hide resolved
@agnes512
Copy link
Contributor Author

The user also suggested that lb4 should allow the project to be generated in the CWD. However, that could be messy and confusing.

I agree. Can the user type . for his case?

Yes, . and .. both work. That's why I mention the CWD in the warning msg. I will make the message more clear.

packages/cli/lib/utils.js Outdated Show resolved Hide resolved
@agnes512 agnes512 force-pushed the lb4app-generate-options branch from 3d91a94 to 3ec96f2 Compare June 26, 2019 19:46
@agnes512 agnes512 requested a review from emonddr June 26, 2019 19:49
@agnes512 agnes512 force-pushed the lb4app-generate-options branch 3 times, most recently from 4f01419 to fdbf542 Compare June 26, 2019 19:57
packages/cli/test/unit/utils.unit.js Outdated Show resolved Hide resolved
packages/cli/test/unit/utils.unit.js Outdated Show resolved Hide resolved
@agnes512 agnes512 force-pushed the lb4app-generate-options branch from fdbf542 to 84c3f70 Compare June 28, 2019 19:23
@agnes512 agnes512 changed the title fix(cli): fix app default project name. check if project path is based on cwd fix(cli): fix app default project name. untildify the project path Jun 28, 2019
@agnes512 agnes512 force-pushed the lb4app-generate-options branch 4 times, most recently from b32c2f4 to 9a3cca9 Compare June 28, 2019 21:27
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM at high level, I'll leave the detailed review to others.

It would be great to add tests to cover the new behavior and prevent regressions in the future.

packages/cli/lib/project-generator.js Outdated Show resolved Hide resolved
packages/cli/lib/project-generator.js Outdated Show resolved Hide resolved
@agnes512 agnes512 force-pushed the lb4app-generate-options branch 3 times, most recently from c16fe6b to 7db70e1 Compare July 4, 2019 17:41
packages/cli/lib/utils.js Outdated Show resolved Hide resolved
packages/cli/test/acceptance/app-run.acceptance.js Outdated Show resolved Hide resolved
packages/cli/test/acceptance/app-run.acceptance.js Outdated Show resolved Hide resolved
@agnes512 agnes512 force-pushed the lb4app-generate-options branch from 7db70e1 to 9a65d61 Compare July 4, 2019 20:26
@agnes512 agnes512 force-pushed the lb4app-generate-options branch 4 times, most recently from 8783ac7 to cf231c8 Compare July 10, 2019 16:12
Copy link
Contributor

@raymondfeng raymondfeng left a comment

Choose a reason for hiding this comment

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

@agnes512 agnes512 force-pushed the lb4app-generate-options branch 2 times, most recently from 9ed5116 to 99a2361 Compare July 10, 2019 18:04
@agnes512 agnes512 force-pushed the lb4app-generate-options branch 14 times, most recently from 3631bd8 to 5c311fd Compare July 15, 2019 13:07
@agnes512 agnes512 force-pushed the lb4app-generate-options branch from 5c311fd to 174052f Compare July 15, 2019 15:04
@agnes512 agnes512 merged commit 7f7feaa into master Jul 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the lb4app-generate-options branch July 15, 2019 15:30
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.

CLI issues: Generated app default Project name invalid if CWD name has hyphens + why not generate in CWD
6 participants