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

polish: improve consistency of warnings and errors #848

Merged
merged 4 commits into from
Apr 27, 2022

Conversation

petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Apr 26, 2022

Related to #377

Screenshot 2022-04-26 at 20 26 30

@changeset-bot
Copy link

changeset-bot bot commented Apr 26, 2022

🦋 Changeset detected

Latest commit: c5e7ba6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Apr 26, 2022

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2232298793/npm-package-wrangler-848

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/848/npm-package-wrangler-848

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2232298793/npm-package-wrangler-848 dev path/to/script.js

@threepointone
Copy link
Contributor

Ooh that tower of icons looks a bit imposing, no?

@JacobMGEvans
Copy link
Contributor

JacobMGEvans commented Apr 26, 2022

Ooh that tower of icons looks a bit imposing, no?

I think this could be refined, but I don't feel it is imposing. Possible layout and formatting?

----------------------------

⚠️ WARNINGS: 
- stuff 
- more stuff 

-----------------------------

❌ ERRORS/DEPRECATIONS: 
- bad stuff
- more bad stuff

@petebacondarwin
Copy link
Contributor Author

Ooh that tower of icons looks a bit imposing, no?

I actually like how it clearly delineates the warnings and errors from the informative messages, which have no icon.
In most cases there should be no errors, so you would not see any Xs.
In many cases there should be no warnings. And if there are it is nice to know where the warning ends and the other messages start.

@petebacondarwin
Copy link
Contributor Author

petebacondarwin commented Apr 26, 2022

The layout suggested by @JacobMGEvans could only be generated if we captured the calls to logger and only rendered them at some point (?) once they had all been completed.
We already do this, effectively, with validation warnings and errors because they are captured in Diagnostics messages, which are then all rendered together after validation completes.

We could add headers to the messages output for validation, though...

@petebacondarwin
Copy link
Contributor Author

I updated to use the esbuild formatter. I am not sure it is any better but open to opinions...

Screenshot 2022-04-26 at 20 26 30

@threepointone
Copy link
Contributor

I like this better!

@petebacondarwin
Copy link
Contributor Author

OK let's go with this. I just need to work out how to fix the Windows snapshots 😿

@petebacondarwin petebacondarwin force-pushed the logger-2 branch 2 times, most recently from 4be5149 to 9c06610 Compare April 27, 2022 09:51
@petebacondarwin petebacondarwin merged commit 0a79d75 into cloudflare:main Apr 27, 2022
@petebacondarwin
Copy link
Contributor Author

I will follow up with a PR to change the mocking to mock the logger rather than the console

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.

3 participants