Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic debug logging #5541

Merged
merged 1 commit into from
Aug 7, 2024
Merged

Basic debug logging #5541

merged 1 commit into from
Aug 7, 2024

Conversation

zoeyTM
Copy link
Contributor

@zoeyTM zoeyTM commented Jul 23, 2024

  • added basic debug logging throughout the main CLI function
  • added a withDebugLogs decorator to core to easily enable debug logging to any class method
    • added this decorator to some class methods that I thought would be helpful, but it can always be added to others in the future as well

fixes #5363

I would add a screenshot of the output, but there's A Lot. Recommend pulling the branch and testing inside v-next/example-project

@zoeyTM zoeyTM added the v-next A Hardhat v3 development task label Jul 23, 2024
Copy link

vercel bot commented Jul 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 30, 2024 6:15am

Copy link

changeset-bot bot commented Jul 23, 2024

⚠️ No Changeset found

Latest commit: a3e153b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@ChristopherDedominici ChristopherDedominici left a comment

Choose a reason for hiding this comment

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

LGTM (I saw the comment about the issue with tsx)

I love the idea of the decorator!
One suggestion: what about a class level decorator that will automatically log for all the methods of the class? So you only have to add it at the beginning instead of every method

@alcuadrado
Copy link
Member

I've been testing this, and I think this is too verbose to be useful. I believe the decorator approach will lead to printing too much data, so I think it's better to remove it and manually log what we find useful.

For example, I think logging these things is useful, but they could be much more succinct:

  • running hooks in the hook manager, which one is run and how.
  • The HRE being created, maybe not even with the config. But knowing that we are creating an HRE can be helpful to debug potential errors (e.g. creating multiple of them by accident)
  • Running a task, but maybe not creating/inserting/fetching them (as that's really verbose)

try {
const usedCliArguments: boolean[] = new Array(cliArguments.length).fill(
false,
);

log("Parsing builtin global options");
Copy link
Member

Choose a reason for hiding this comment

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

I think printing just the results would probably be enough in most cases, and make the output less verbose/more helpful.

Copy link
Member

Choose a reason for hiding this comment

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

I mean in general. I noticed a pattern of "doing this", "this is its result", which I do a ton for manual debugging, but can be too verbose.

Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

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

Just left a few comments

@kanej kanej removed their request for review August 6, 2024 09:08
@schaable schaable removed their request for review August 6, 2024 16:42
@zoeyTM zoeyTM merged commit 272ef05 into v-next Aug 7, 2024
51 checks passed
@zoeyTM zoeyTM deleted the debug-logs branch August 7, 2024 10:30
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
v-next A Hardhat v3 development task
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants