pkgjs / wiby

"Will I break you" - a tool for testing dependents
Apache License 2.0
33 stars 7 forks source link

Pretty print results #57

Open dominykas opened 3 years ago

dominykas commented 3 years ago

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

rodion-arr commented 3 years ago

Could you please specify more precisely what are requirements here?

dominykas commented 3 years ago

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:

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

rodion-arr commented 3 years ago

Yeah will be happy to help here, assigning to myself

ljharb commented 3 years ago

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

rodion-arr commented 3 years ago

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 commented 3 years ago

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:

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):

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 commented 3 years ago

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