search

tweag / topiary

https://topiary.tweag.io/
MIT License
511 stars 17 forks source link

Distinguish library and CLI log messages #666

Closed evertedsphere closed 5 months ago

evertedsphere commented 6 months ago

Does both things suggested in #604, by

  1. renaming the topiary CLI binary to topiary-cli
  2. adding a LevelFilter specific to messages from topiary::* modules (i.e. the library)

In this PR in its current state, the levels are inferred heuristically from the existing command-line verbosity setting. Both the particular mapping I came up with semi-randomly and the appropriateness of doing this rather than adding a second e.g. --lib-log-level flag are up for debate.

Something more important stems from the fact that renaming the library in topiary-cli/Cargo.toml (i.e. 

topiary-core = { package = "topiary" }

etc) doesn't seem to work. The module names in the logs seem to depend on the names of modules as they are defined, so to pursue this route we'd have to rename the library crate itself to topiary-core or some such. I personally would prefer that approach, as it's idiomatic Rust and avoids changing the name of the executable, but it's something that needs to be discussed first and can easily be added to this PR after. If merged as it is, this PR will require changes to release workflows to account for the change in the name of the Topiary binary.

Xophmeister commented 5 months ago

@ErinvanderVeen It would be a bit cheeky of me to approve this PR myself :wink: Could you give it a yea/nay, please?

Per the discussion above, I have renamed the library to topiary-core and made all the necessary adjustments. That allows the logging output to be distinguishable from the library (topiary_core) and the CLI (topiary_cli).

I've also taken the opportunity to rename the top-level directories to things like core and cli, to make navigation easier (for lazy people like me!)

The CI will fail, but the meat of it all works fine. The failure is per #665 and unrelated to the changes made here. I think that can be ignored for the sake of this PR.

Xophmeister commented 5 months ago

I have reverted the short-form directory names, so this is ready to merge (after #672)

Xophmeister commented 5 months ago

...Oh, of course, there's some CI problem :roll_eyes:

Xophmeister commented 5 months ago

Just a simple formatting problem...fixed. Now it is ready for merge, once approved.

Xophmeister commented 5 months ago

This is ready now, I just need a :heavy_check_mark: @ErinvanderVeen

(It's still failing #665, but we can look into that elsewhere.)

evertedsphere commented 5 months ago

Thanks for seeing this through.