Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Default to concise logging. Support --name and --repo flags for skyux new. #158

Merged
merged 7 commits into from
Nov 28, 2018

Conversation

Blackbaud-BobbyEarl
Copy link

No description provided.

@codecov-io
Copy link

codecov-io commented Aug 11, 2018

Codecov Report

Merging #158 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #158   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           8      8           
  Lines         265    292   +27     
  Branches       40     45    +5     
=====================================
+ Hits          265    292   +27
Impacted Files Coverage Δ
lib/npm-install.js 100% <100%> (ø) ⬆️
lib/new.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47e58f9...c95adac. Read the comment docs.

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Assuming we only want to add this feature to the CLI (and not @blackbaud/skyux-logger) this looks and works great.

@Blackbaud-BobbyEarl
Copy link
Author

Hey @Blackbaud-SteveBrush, I'm overloading this PR. I just updated to add support for --name and --repo to bypass the prompts. The --name still passes through validation.

@Blackbaud-BobbyEarl
Copy link
Author

I thought about your question @Blackbaud-SteveBrush. My vote would be to introduce it here as a one-off. If we find we like it, or actually have a need for it, it would be easy enough to move that logic into the logger.

@Blackbaud-BobbyEarl Blackbaud-BobbyEarl changed the title Using the ora library for logging (unless verbose) Default to concise logging. Support --name and --repo flags for skyux new. Nov 16, 2018
@Blackbaud-BobbyEarl
Copy link
Author

As first suggested, @Blackbaud-SteveBrush, I've moved the ora logic into a new logger.promise method in @blackbaud/skyux-logger.

This PR should be updated to point to that released version, as I'm temporarily pointing to the branch.

'SPA root directories may only contain lower-case letters, numbers or dashes.'
);
throw new Error();

Choose a reason for hiding this comment

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

Is throwing an empty error a requirement to fail the process? Could we not process.exit(1)? Just seemed odd to me.

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush Nov 27, 2018

Choose a reason for hiding this comment

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

Also, my console printed the following:

$ skyux new --name FOOBAR
SKY UX processing command new
SPA root directories may only contain lower-case letters, numbers or dashes.
ERROR MESSAGE

See "ERROR MESSAGE". It also proceeded to the next step of the process instead of exiting. Is that accurate?

Choose a reason for hiding this comment

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

The ERROR MESSAGE was a rogue console.log hanging out. I'll remove that silly thing.

When you say "proceeded to the next step," can you confirm you see "What is the root directory for your SPA? (example: my-spa-name)" displayed. That's actually the first step, which is bypassed by passing in the name argument.

Choose a reason for hiding this comment

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

The throw new Error() is necessary to have the validator of the prompt library fail.

@Blackbaud-SteveBrush Blackbaud-SteveBrush dismissed their stale review November 27, 2018 18:32

More changes were added since the last review.

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Feature works great. Aside from our conversation about the "name" vs "root directory" messaging, this LGTM.

@blackbaud-johnly
Copy link
Contributor

Created blackbaud/skyux2#2195 to document the new flags

@Blackbaud-BobbyEarl Blackbaud-BobbyEarl merged commit 72d3b6d into master Nov 28, 2018
@Blackbaud-BobbyEarl Blackbaud-BobbyEarl deleted the using-consolidated-logging branch November 28, 2018 20:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants