Add dependency name in the logs #676
Conversation
miry
force-pushed
the
logging-context-package-name
branch
from
ba3b6ce to
43c1fca
Compare
In certain cases, it can be unclear which debug log message is associated with which dependency, where logs for multiple dependencies may appear in different order. These changes improve log clarity by explicitly including the package name in relevant messages.
miry
force-pushed
the
logging-context-package-name
branch
from
43c1fca to
0e745bf
Compare
src/package.cr
Outdated
| cleanup_install_directory | ||
| Log.with_context do | ||
| Log.context.set package: name | ||
| Log.context.set version: report_version |
There was a problem hiding this comment.
I added this for debugging purposes. I haven’t identified a use case for regular users yet, but I plan to use it to detect discrepancies between the expected and installed versions.
Let me know if I should keep it or not.
There was a problem hiding this comment.
Not sure if it's relevant.
But if we keep it, we should merge both into a single context assignment for efficiency.
Also, is there any reason you're using explicit set calls instead of passing the data to with_context? That'd be more concise.
There was a problem hiding this comment.
Also, is there any reason you're using explicit set calls instead of passing the data to with_context? That'd be more concise.
Good call - because the with_context(**kwargs) was introduced after crystal 1.0.0. I found there are still tests are runing against Crystal 1.0.0 that are failing. matrix
I thought to introduce patch for older version to support with_context(**kwargs).
Example of the failing step: https://github.com/miry/shards/actions/runs/15099018619/job/42437500620
There was a problem hiding this comment.
Okay, that's a good reason 😆
I like the idea of adding a polyfill for Crystal < 1.3.
However, it would be fine to keep a separate set call either.
There was a problem hiding this comment.
Some of these context assignment locations look odd.
The package context should be set when entering the scope of a specific package (e.g. in each interaction when iterating over some packages).
But often they're just an instance method on a resolver. I figure we should perhaps move some of these to the caller scopes.
|
Yeah, I have introduced some It made me wonder whether I should introduce more logging steps for debugging purposes or remove those extra context statements. |
|
@straight-shoota I have updated code a bit. It should have less context calls. |
In certain cases, it's unclear which package causes an error during shard installation. These changes improves log messages by adding explicit package context.
The log will now indicate which package triggered the event.
Tophat