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

misc(logger): add warn level #14964

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Conversation

doteric
Copy link
Contributor

@doteric doteric commented Apr 12, 2023

Summary

There is a missing log level between error and info which is simply warn to show just errors and warnings and nothing else. Very small and simple change required to add some better experience.

@google-cla
Copy link

google-cla bot commented Apr 12, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@connorjclark connorjclark changed the title misc: add warn log level misc(logger): add warn level Apr 17, 2023
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

Some failing tests to address FYI

@doteric
Copy link
Contributor Author

doteric commented Apr 17, 2023

@connorjclark
https://github.com/GoogleChrome/chrome-launcher/blob/main/src/chrome-launcher.ts#L17
https://github.com/GoogleChrome/chrome-launcher/blob/main/src/chrome-launcher.ts#L38

That's pretty odd typing a little bit. Any suggestions what should be done about it?
lighthouse-logger needs to be updated if we want to change it in chrome-launcher otherwise it wouldn't make sense...
IMO. the typing is broken.

@connorjclark
Copy link
Collaborator

connorjclark commented Apr 17, 2023

If you can send a PR to update chrome-launcher in the same way, we can update all at once.

Another option is to add // @ts-expect-error explaniation in the place where typescript is erroring in the lighthouse repo.

doteric added a commit to doteric/chrome-launcher that referenced this pull request Apr 17, 2023
@doteric
Copy link
Contributor Author

doteric commented Apr 17, 2023

@connorjclark
GoogleChrome/chrome-launcher#295
But I think it might be worth removing that type from chrome-launcher and infer from lighthouse-logger in the future 🤔

connorjclark pushed a commit to GoogleChrome/chrome-launcher that referenced this pull request Apr 17, 2023
@connorjclark
Copy link
Collaborator

Thanks @doteric!

@doteric doteric deleted the warn-level-log branch April 18, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants