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

feat: add reporter layer to support JSON output #97

Merged
merged 2 commits into from Apr 27, 2019

Conversation

Projects
None yet
2 participants
@vostrik
Copy link
Contributor

commented Apr 27, 2019

Added JSON output with --json argument.

Also added reporter layer to extend in future base reporters. It can be useful in integrations to such tools like Teamcity, etc.

#94

@ai

This comment has been minimized.

Copy link
Owner

commented Apr 27, 2019

I like your architecture. But let’s reduce the number of classes. JsonReporter and HumanReporter will be enough.

runner.js Outdated
}

process.stdout.write('\n' + output + '\n')
reporter.logResults({ results, hint })

This comment has been minimized.

Copy link
@ai

ai Apr 27, 2019

Owner

You do not need objects here. Just use arguments reporter.logResults(results, hint)

This comment has been minimized.

Copy link
@vostrik

vostrik Apr 27, 2019

Author Contributor

Done in all methods

runner.js Outdated
@@ -486,6 +494,8 @@ main().catch(e => {
msg = e.stack
}

process.stderr.write(`${ chalk.bgRed(' ERROR ') } ${ chalk.red(msg) }\n`)
reporter.error({
message: `${ chalk.bgRed(' ERROR ') } ${ chalk.red(msg) }\n`

This comment has been minimized.

Copy link
@ai

ai Apr 27, 2019

Owner

Should we move ERROR to a reporter?

This comment has been minimized.

Copy link
@vostrik

vostrik Apr 27, 2019

Author Contributor

Yeah, I think so. Also in warn method?

This comment has been minimized.

Copy link
@ai

ai Apr 27, 2019

Owner

Yeap

This comment has been minimized.

Copy link
@vostrik

vostrik Apr 27, 2019

Author Contributor

Done for warn() and error() in HumanReporter

}
}

module.exports = {

This comment has been minimized.

Copy link
@ai

ai Apr 27, 2019

Owner

We can directly export class here

This comment has been minimized.

Copy link
@vostrik

vostrik Apr 27, 2019

Author Contributor

Ok.
Do you think that named export to save naming consistence in this case is bad practice?

This comment has been minimized.

Copy link
@ai

ai Apr 27, 2019

Owner

I am exporting function directly without object wrap https://github.com/ai/size-limit/blob/master/index.js#L255

So for consistency, we should do the same with new files.

This comment has been minimized.

Copy link
@vostrik

vostrik Apr 27, 2019

Author Contributor

Done for all new modules

@ai ai merged commit 44969eb into ai:master Apr 27, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ai ai referenced this pull request Apr 27, 2019

Closed

Add JSON argument for CLI #96

@ai

This comment has been minimized.

Copy link
Owner

commented Apr 27, 2019

I simplified code even more 27891e6

@ai ai referenced this pull request Apr 27, 2019

Closed

Output to file #94

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.