Make corral less verbose

[hasheddan]

This updates most non-log prints to stdout to have a log level of fine
such that they will only be displayed when the --verbose flag is passed.
Some commands are ommitted from this change, such as info, version, and
list as their explicit purpose is to print information.

Fixes #101

Signed-off-by: hasheddan georgedanielmangum@gmail.com

[hasheddan]

Noticed #133 after opening this. I am happy to hold and rebase until after that is merged, or scrap this PR if that one intends to address this issue as well 👍

[hasheddan]

Looks like tests will also need to be updated as they are evaluating the old output

[SeanTAllen]

I would say, make the tests work with new output and let's start a discussion in the #tooling stream on zulip to find better test to replace the brittle output tests.

[SeanTAllen]

@hasheddan #133 is unrelated.

[hasheddan]

@SeanTAllen I just updated the failing tests to run with --verbose as a short-term solution

[SeanTAllen]

Seems like a good idea.

Can you open an issue that details which tests are using the output so we can start working out replacements?

[hasheddan]

@SeanTAllen sure thing!

[SeanTAllen]

Rather than changing the log level to fine, I think that by default only verbosity with existing logger should be LvlErrr, you'd have to ask for anything lower than that to be displayed.

And when #133 is done, the default logger level would be Error.

This would fit with the general command line tool idea of "by default, only print errors".

@Theodus thoughts?

[hasheddan]

@SeanTAllen that makes sense 👍 My reasoning for changing to fine here was due to the fact that commands like version are being passed the same logger for writing to stdout, and we want those to print info level by default (as their purpose is printing information to the user). Agree this is a bit of a rough workaround to make that happen

[hasheddan]

Dropping a note here from Pony sync call this week: instead of changing these log levels across the board, we could pass a different logger for "informational commands" and let --verbose behave differently on those. That would mean that non-informational commands would not print anything unless --verbose was supplied, and informational commands would print whether --verbose was supplied or not. Note that for informational commands that would mean providing --verbose would have mostly no effect.

Any thoughts on this @SeanTAllen?

[SeanTAllen]

What's an informational command?

[hasheddan]

version, info, list, etc.. any command that has an explicit purpose of printing information

[SeanTAllen]

So fetch, init, update are "non-informational", anything else non-informational?

[hasheddan]

I would say remove, add, clean, run would be categorized as non-informational.

[SeanTAllen]

@hasheddan This sounds very reasonable.

[hasheddan]

@SeanTAllen nice! I will update and ping you later this evening when ready for another review 👍

[hasheddan]

@SeanTAllen Did a quick refactor here. It seems to fix the problem we are trying to address, but I don't think it is our ultimate final state. Let me know if you want additional work here before merge :)

[hasheddan]

Not sure how elegant this is in Pony vernacular. Seems like it might be worth refactoring the Executor in the future, but this felt like the lower friction move for now. Happy to adjust if you prefer another pattern 👍

[SeanTAllen]

@hasheddan My statement: it fixes a problem. Yeah, we don't want this as a final state. I think a better "closer to final" state is for each command "carry" the "logger" with it.

Commands are really anemic at this point. I think it makes sense for when a command is created for this log level to be set.

Perhaps there is a trait for an informational vs action command that each command will implement and it can then somehow "select" the correct logging.

Otoh, that might be really ugly.

Can you open an issue for this to be cleaned up more?

Rather than "just fixing" that issue, I'd like to see it done as part of writing tests to replace the ones that you had to modify to make this pass. I think if we have tests like

"Fetch will only output if X" and "Fetch won't output when Y" then we will end up at a good solid testable design, does that sound good to you? If yes, I'm good with merging this once you have opened that issue/updated #138 to incorporate these additional notes.

[hasheddan]

@SeanTAllen done 👍 #140

[SeanTAllen]

Thanks @hasheddan!