-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Move date.now and console.time audits to table formatter #1547
Conversation
what about something simple like text-table with no deps and tiny in size. |
case 'lineCol': | ||
return `${result.line}:${result.col}`; | ||
case 'isEval': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of accumulating audit-specific display logic here, do we anticipate isEval
cropping up in several other places too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a few JS audits that this is relevant for.
thoughts on using |
I'd like to prettify the pretty print in another PR, but don't we need a module that's checked in to g3? |
@patrickhulce should we |
New modules can be checked in, it's just a big responsibility..."we" have to own compat and maintenance. Since it has no dependencies it makes that a lot easier since it'll only be the addition of a single module. |
yeah, there are a few with a lot more features, but e.g. table-layout brings in seven other modules in addition to itself. I guess it just depends on how much we need fancy table layout. cli-table is another option, and only brings in colors, but the compatibility there is harder as it's fixed on 1.03, we already have |
I like it in theory, but there are some complexities with inline/previews we have to consider and I was going to suggest accepting a parameter for the base URL so that cross-origin hosts would be shown, since those weren't so obvious for me on a few runs which would complicate the formatter a bit. Either way let's save for another PR? |
SG |
table formatter test bugs on travis |
fixed tests! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
R: all
This also ads the table heads to the pretty print output. The alignment is not great, but otherwise it's hard to know what the columns mean. Ideally, we'd bring in a CLI table formatter, but ...g3 :(