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

Pass comments generated by lookout-tool review call to stdout #601

Closed
zurk opened this issue Mar 26, 2019 · 9 comments · Fixed by #624
Closed

Pass comments generated by lookout-tool review call to stdout #601

zurk opened this issue Mar 26, 2019 · 9 comments · Fixed by #624
Assignees
Labels
enhancement New feature or request

Comments

@zurk
Copy link

zurk commented Mar 26, 2019

Right now I need to parse the lookout-tool logs to get Analyzer comments and rely on internal implementation logic (https://github.com/src-d/lookout/blob/master/server/server.go#L559).
For example something like
go run cmd/lookout-sdk/*.go review --log-format=json 2>&1 | jq 'select(.msg == "file comment")' can work.

I think that comments can be interpreted as lookout-tool review output and passed to stdout.
It can be in the same log format (JSON/not JSON).

So in ideal case, I just want to run
go run cmd/lookout-sdk/*.go review --log-format=json > comments.json and get comment per line in JSON format.

@carlosms carlosms added the enhancement New feature or request label Mar 26, 2019
@smacker smacker self-assigned this Mar 29, 2019
@smacker
Copy link
Contributor

smacker commented Mar 29, 2019

There is a limitation in go-log: src-d/go-log#11
Go-log doesn't have a maintainer to give feedback/review/merge so I just return this issue to TODO.

@smacker smacker removed their assignment Mar 29, 2019
@carlosms
Copy link
Contributor

carlosms commented Apr 4, 2019

ping @smola so you are aware of the missing maintainer issue.

@carlosms carlosms added the blocked Can not be started or continued for external limitations label Apr 5, 2019
@smola
Copy link
Contributor

smola commented Apr 5, 2019

We actually might want to have a JSON poster, alternative to the log one. As we discussed recently, to enable different evaluation and testing use cases. So logs would still go to stderr, but results can be posted as JSON to stdout.

@carlosms carlosms removed the blocked Can not be started or continued for external limitations label Apr 5, 2019
@smacker
Copy link
Contributor

smacker commented Apr 5, 2019

@smola what is the difference between JSON poster and log poster with --format=json?

@smola
Copy link
Contributor

smola commented Apr 5, 2019

@smacker Logs are written to stdout (no matter the format), while JSON poster would post to stdout. It's kind of differentiating between logging and output. For example: lookout ... > output.json would still output logs either in JSON or human readable, but post the meat of the review to the file.

@smola
Copy link
Contributor

smola commented Apr 5, 2019

But it's true that the log poster could be used, just with a different setup (JSON + stdout) than the general logger?

@smacker
Copy link
Contributor

smacker commented Apr 5, 2019

We have LogPoster struct which implements Poster interface.
The constructor of it requires go-log.Logger instance. It can be a general one or a separate one.

main.go decides what to pass there.
Currently, it's log.DefaultLogger but the initial solution for this issue was just to pass log.New().Output(os.Stdout) (it would use DefaultFactory which is set by go-cli)

This way:

  • we don't duplicate code to do the same posting
  • results of analyzer go to stdout always, no matter does user prefer json format or not. Which makes it easy to pipe. For example to test that an analyzer returned N comments you just need to do ./lookout-sdk review | wc -l. Now this command returns 0 always which doesn't look correct to me.

@dpordomingo
Copy link
Contributor

If I'm not wrong, this requirement would be satisfied by #624
(and wc -l would also work)

$ lookout-sdk review --from HEAD^ 2> log.err | wc -l
5

$ lookout-sdk review --from HEAD^ 2> log.err
{"analyzer-name":"","file":"cmd/lookout-sdk/push.go","text":"The file has increased in 2 lines."}
{"analyzer-name":"","file":"cmd/lookout-sdk/push.go","line":27,"text":"This line exceeded 120 chars."}
{"analyzer-name":"","file":"cmd/lookout-sdk/review.go","text":"The file has increased in 2 lines."}
{"analyzer-name":"","file":"cmd/lookout-sdk/review.go","line":23,"text":"This line exceeded 120 chars."}
{"analyzer-name":"","file":"cmd/sdk-test/bblfsh_test.go","line":146,"text":"This line exceeded 120 chars."}

@dpordomingo dpordomingo self-assigned this Apr 9, 2019
@smacker
Copy link
Contributor

smacker commented Apr 9, 2019

Imo by default lookout-sdk review should continue print normal response and switch to the json only of the user asks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants