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

feat: pure console.logs #374

Merged
merged 1 commit into from May 8, 2023
Merged

feat: pure console.logs #374

merged 1 commit into from May 8, 2023

Conversation

0xdapper
Copy link
Contributor

@0xdapper 0xdapper commented May 7, 2023

h/t @0age https://twitter.com/z0age/status/1654922202930888704

Thought about creating a different console3.sol but there is no downside to just making everything pure.

@mds1
Copy link
Collaborator

mds1 commented May 7, 2023

Awesome, thank you! Was hoping someone would PR this, really clever workaround

I'll take a closer look tomorrow or monday before merging, but can we revert the changes to console.sol? That one is left untouched to ensure compatibility with hardhat, whereas console2.sol fixes the original selector issues of console.sol + we can modify/extend it for forge as needed

@0xdapper
Copy link
Contributor Author

0xdapper commented May 7, 2023

I don't think this should break the compatibility with hardhat

@mds1
Copy link
Collaborator

mds1 commented May 8, 2023

I also don't think it should break the compatibility with hardhat, but I haven't tested to confirm, so would prefer to leave it unless there's a reason to change it. Not sure who uses console.sol or what assumptions hardhat makes about it. I guess one downside of changing it is that now anyone using console.sol in hardhat projects with minimal forge usage now get compiler warnings about function visibility 🤷 (which of course will happen in forge projects using forge's console2 also)

Aside from that this PR lgtm

Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

looks great, thank you 🫡

Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

looks great, thank you 🫡

@mds1 mds1 merged commit 7f7cb06 into foundry-rs:master May 8, 2023
3 checks passed
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.

None yet

2 participants