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 Project Names #468

Merged
merged 3 commits into from
Jan 21, 2018
Merged

Validate Project Names #468

merged 3 commits into from
Jan 21, 2018

Conversation

milesthedisch
Copy link
Contributor

What kind of change does this PR introduce?

Add test and validation to dest and/or name options.

Did you add tests for your changes?

Yes

Summary

#466

Does this PR introduce a breaking change?

No

Other information

  • node v6.10.3
  • npm 3.10.10
  • yarn 1.3.2
  • macOS Sierra 10.12.6

@milesthedisch milesthedisch changed the title https://github.com/developit/preact-cli/issues/466 Address's #466 Jan 20, 2018

if (npmPackageNameErrors) {
const npmPackageNameMsg = npmPackageNameErrors.join(', ');
return error(`${argv.name} is not a valid package name. ${npmPackageNameMsg}`, 1);
Copy link
Member

Choose a reason for hiding this comment

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

can you add a chalk message before this?

Copy link
Contributor Author

@milesthedisch milesthedisch Jan 21, 2018

Choose a reason for hiding this comment

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

Not sure what the chalk message would have in it. Doesn't the error method from the utils already display the message given?

i.e ✖ ERROR ./asdasd is not a valid package name. name cannot start with a period, name can only contain URL-friendly characters

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Internally error method in utils uses chalk itself.

@prateekbh that is totally valid. 😄

Copy link
Member

Choose a reason for hiding this comment

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

gosh I need to do my revision for this codebase

@ForsakenHarmony ForsakenHarmony changed the title Address's #466 Validate Project Names Jan 20, 2018
Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

This looks good! My only suggestion is formatting on that error message:

if (npmPackageNameErrors) {
  const msgs = [`Invalid package name: ${argv.name}`].concat(npmPackageNameErrors);

  return error(msgs.map(capitalize).join('\n  >'), 1);
}
✖ ERROR Invalid package name: ./asdasd
  > Name cannot start with a period
  > Name can only contain URL-friendly characters

We can totally get this in a separate PR tho. 👍

@lukeed lukeed merged commit 12648dc into preactjs:master Jan 21, 2018
@lukeed
Copy link
Member

lukeed commented Jan 21, 2018

Thank you @milesthedisch ~!

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.

4 participants