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

Console log #284

Merged
merged 6 commits into from
Aug 18, 2020
Merged

Console log #284

merged 6 commits into from
Aug 18, 2020

Conversation

chenlevy24
Copy link
Contributor

Changed Log into an interface and added ConsoleLog

Copy link
Collaborator

@PEConn PEConn left a comment

Choose a reason for hiding this comment

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

This was so much easier to review! Thanks!

packages/cli/src/index.ts Outdated Show resolved Hide resolved
Comment on lines 50 to 51
"devDependencies": {
"@types/mime-types": "^2.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @andreban made a change that moved all the dev dependencies somewhere - ad139d1 .

I'm not sure if this goes against that, but lets see what he says.

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 revert this change, please. mime-types is needed as a runtime dependency. This is also downgrading the minimum version from 2.1.27 to 2.1.0. Generally, devDependencies should be empty in core, cli and validator and listed on the root package.json.

debug(message: string, ...args: string[]): void;

/**
* Prints an info message to the Log. message is ignored if the Log is not set to verbose.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These comments seem to be wrong (the bit about verbose only applies to debug I think).

Also, capital letters at the start of sentences please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't write it, and I think it is right, if verbose == false it will not print anything

Copy link
Collaborator

@PEConn PEConn left a comment

Choose a reason for hiding this comment

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

LGTM once you get rid of the import Log that you don't use (that GitHub Actions is warning you about).

(Also once Andre is happy with it.)

Copy link
Member

@andreban andreban left a comment

Choose a reason for hiding this comment

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

Thanks, this is much easier to review. Requested one change.

Comment on lines 50 to 51
"devDependencies": {
"@types/mime-types": "^2.1.0"
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 revert this change, please. mime-types is needed as a runtime dependency. This is also downgrading the minimum version from 2.1.27 to 2.1.0. Generally, devDependencies should be empty in core, cli and validator and listed on the root package.json.

packages/core/src/lib/Log.ts Outdated Show resolved Hide resolved
"inquirer": "^7.3.2",
"jimp": "^0.14.0",
"inquirer": "^7.0.4",
"sharp": "^0.23.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to change this line?

Copy link
Collaborator

@PEConn PEConn left a comment

Choose a reason for hiding this comment

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

Cool, looking good to me.

Copy link
Member

@andreban andreban left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM, thank you for the hard work!

@andreban andreban merged commit 6245939 into GoogleChromeLabs:master Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants