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

Pretty print results #57

Open
dominykas opened this issue Oct 27, 2020 · 7 comments
Open

Pretty print results #57

dominykas opened this issue Oct 27, 2020 · 7 comments
Assignees
Labels
enhancement New feature or request

Comments

@dominykas
Copy link
Member

At the moment wiby result just dumps some json - we can do better than that (although --json would still be useful!)

@rodion-arr
Copy link
Member

Could you please specify more precisely what are requirements here?

@dominykas
Copy link
Member Author

I don't know :) I'd normally iterate on this, because there's things this needs figured out, which I'm not certain about, namely GH has two status APIs (checks and statuses) and I'm not very familiar with the checks one. I'm also not sure what's the future of the statuses one - is it getting deprecated or smth? Maybe there are some resources or at least discussions on some GH boards about that?

But the goal here is that it should:

  • print a nicely formatted table for CLI (which lists all the checks for all the repos and their statuses)
  • print a nicely formatted table in markdown (not critical for the first iteration, but this is for posting/updating a comment on a PR)
  • report the same in json

Would you be willing to do some research and make some proposals, in code or otherwise?

@rodion-arr
Copy link
Member

Yeah will be happy to help here, assigning to myself

@rodion-arr rodion-arr self-assigned this Nov 2, 2020
@ljharb
Copy link
Member

ljharb commented Nov 2, 2020

Statuses isn't deprecated afaik, but using "checks" gives a much nicer experience.

@rodion-arr
Copy link
Member

Regarding formatting as I see we are going to support multiple output formats. What about adding new -o, --output= option with table|md|json values?

I'd like to remove simple console.logs in this case and build separate module with formatting logic in order to avoid multiple if (!args.json) {} like we have in dependents package.
For outputting table to CLI we can use cli-table

Regarding contents - I see wiby uses both APIs: checks as default and statuses as fallback. Are we going to have some changes in this logic in scope of this task?

@dominykas
Copy link
Member Author

dominykas commented Dec 30, 2020

Sorry, I missed this last comment and only noticed after coming back to this since we need a non-zero exit code when there's a failure - might make sense to do as part of this issue?

tl;dr at the end.

Regarding formatting as I see we are going to support multiple output formats. What about adding new -o, --output= option with table|md|json values?

I wonder if that's enough as an option? Pondering the use case of CI - we'd probably want to output something human readable in console and we'll need something machine readable in a file.

I'm also pondering just now that maybe after all we don't need the separate table output - markdown looks good enough in console (and we can sprinkle some colour around it later)?

I'd like to remove simple console.logs in this case and build separate module with formatting logic

I guess there's two concerns about the output here:

  • generic logging as the app progresses to log what it's doing
  • the output of the final result

For the first part - I probably agree that we need a wrapper to replace current console.logs - I figure we may as well use debug? I think a good starting point would be to just introduce it now, and then see if we need anything more complex than that (e.g. taking in a context.logger or something as an arg in the exported methods).

And then the function exported from result.js should just return the normalized result (instead of console.loging it) and it's up to the handler() to decide how to output it?

And maybe this answers the question about the CLI args? We need to be able to control three things (for results, but probably for test as well, because the test output is the same, except everything is pending):

  • --output-json
  • --output-md
  • --output-log

The values for either of these can be true (output to console), false (do not output) and a file name. Defaults: --output-json=false --output-md=false --output-log=true. It should also include the results in the log as md, i.e. having an --output-md=true --output-log=true would output the md twice (however, nobody should use it that way, so it's not a problem). And then as well by default, if --output-json or --output-md are true, then --output-log becomes false automatically. Not sure if this can be expressed gracefully via Yargs API. And then the wiby action that I'm building will likely have --output-json=results.json --output-md=results.md --output-log=true.

Or is this overengineered? 🤔

Just for completeness - I'm aware you can control debug's output via DEBUG env var, but I think the wiby cli should control it explicitly via it's own arg (so that debug can be swapped out, if need be) and when wiby is used as a lib - controlling via the env var makes sense, but we'll want to extend it to take in the context.logger or smth in the future.

Regarding contents - I see wiby uses both APIs: checks as default and statuses as fallback. Are we going to have some changes in this logic in scope of this task?

Yeah, so I'm not too familiar with the purpose of these two APIs. I wish GH would have just created a normalization layer, so that there's only one... I'm not even sure if these are mutually exclusive - can there be some checks and some statuses on the same commit? This probably needs experimentation and ideally it would be documented before we proceed.

But what I do think we need is a sort of abstraction for output / reporting. As a starting point, we'll need a list, where each item is a { dependent, commit_sha, status, details } (naming just as an example, can probably be improved to be more specific), where status is a final, calculated result out of details (arguably, if it's calculated, we don't need it, but it does simplify the life for consumers and is directly printable?). And then details is probably an array with { status, id, url } (not sure whether id or name or both make sense here, but the important bit is the url, so that we can output it markdown under <details/>?


Sorry for the wall of text above, that said, I think for the very very first bit here's what we should do:

  1. The result.js exported function should return { status, results: [{ dependent, status }] }
    • overall status: success, pending, failure
    • results is the status per dependent
  2. wiby result should print whatever is returned from result.js as markdown
  3. wiby result should exit with non-zero code when overall status is not success; it should use different exit codes for failure and pending. https://tldp.org/LDP/abs/html/exitcodes.html proposes restricting user-defined exit codes to the range 64 - 113. Maybe 1 for failure and 64 for pending?
  4. We should replace all console.logs with debug (and change some of the logs to be tagged with debug and only display them on demand via DEBUG env var)

No explicit --output option needed for now?

Does that make sense?

@rodion-arr
Copy link
Member

Sounds good to me, I'll try to start working on this in nearest days

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

No branches or pull requests

3 participants