Skip to content

Conversation

gulshanpr
Copy link

@gulshanpr gulshanpr commented Sep 1, 2025

Title

Standardize error logs to use %e token

Description

Replaced old %s/%o error logging with the %e token across the codebase for consistency.

Issue #3161

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@gulshanpr gulshanpr requested a review from a team as a code owner September 1, 2025 03:46
@seetadev
Copy link

seetadev commented Sep 1, 2025

@gulshanpr: Hi Gulshan. Wish to ask if the PR has been tested locally.

Please attend the upcoming js-libp2p calls on Wednesday. We will work out a clear plan on ensuring we are meeting the requisite needs in reference to js-libp2p. CCing @dhuseby.

@gulshanpr
Copy link
Author

so to prove this PR, do i need to add test along with it or running npm run test locally works?

@achingbrain achingbrain changed the title fix(logging): standardize error logs to use %e token fix: standardize error logs to use %e token Sep 3, 2025
@achingbrain
Copy link
Member

Thanks for opening this PR.

As it just changes log lines there's no need to add a test.

Unfortunately there have been disruptive changes in main as part of prep for the v3 release - please can you fix the merge conflicts here.

@gulshanpr
Copy link
Author

gulshanpr commented Sep 6, 2025

hi @achingbrain i've fixed the merge conflicts, can you review it now?

Copy link
Member

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have done a partial review but this PR is full of merge conflicts that have not been resolved and instead added as-is to the PR.

I cannot continue this review until this PR will apply to main cleanly.

General comments are:

  1. Where there was previously this.log(err), instead of changing it to this.log('%e', err), add some more context, e.g. this.log('error while Xing the Y - %e', err).
  2. Do not change the casing of log messages, in this codebase error logging is lowercase and messages passed to Error instances is sentence case

@gulshanpr
Copy link
Author

Hey @achingbrain,
I fixed all the requested changes and made sure,

  • casing is correct
  • context in log.error()
  • and no more merge conflicts

could you pls review it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants