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

Adds formatters for pretty and report outputs #301

Merged
merged 8 commits into from
May 9, 2016
Merged

Adds formatters for pretty and report outputs #301

merged 8 commits into from
May 9, 2016

Conversation

paullewis
Copy link
Contributor

@paullewis paullewis commented May 9, 2016

Works with a11y audit atm, but this change will allow support for CRP etc.

class Accessibilty extends Formatter {
static getPrettyFormatter() {
return function(info) {
let output = ` - Rating: ${info.impact}\n`;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just use a multiline string for the initial assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@samccone samccone added the +1 label May 9, 2016
return this._formatters[name];
}

static getPrettyFormatter() {
Copy link
Member

Choose a reason for hiding this comment

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

shoudl we do 1:1 matching against our --output value options? so maybe getFormatter('pretty')

Copy link
Member

Choose a reason for hiding this comment

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

what is the story for JSON output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulirish The formatter matches the internal type, e.g. 'critical-render-path', 'accessibility', 'security'... it's essentially the formatter to use. Even if we 1:1 it, we'd still hard code the ReportGenerator to request HTML, and the CLI to request pretty, so I dunno we'd get much benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The story for JSON is that we output the extendedInfo as-is.

@paulirish paulirish added the +2 label May 9, 2016
@paulirish
Copy link
Member

Seems good. Needs a rebase

@paullewis paullewis merged commit fe405e5 into GoogleChrome:master May 9, 2016
@paullewis paullewis deleted the formatters branch May 9, 2016 18:55
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