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: add format parameter to hardhat-abi-exporter #42

Merged
merged 3 commits into from Jul 3, 2022

Conversation

thelostone-mc
Copy link
Contributor

@thelostone-mc thelostone-mc commented Jul 2, 2022

Description

  • updated documentation to add new parameter -> format
  • set default format to json
  • supported formats are json, fullName, minimal

@thelostone-mc
Copy link
Contributor Author

Hey @ItsNickBarry , i was using your plugin to generate human readable ABI and realized this package supports only JSON and minimal. I've tweaked the parameter but this does like a breaking change and I wasn't sure if this is something you were considering.

https://docs.ethers.io/v5/api/utils/abi/formats/

If you find this alright, would love to get feedback + have this merged.
If you feel that this breaking change isn't needed at this point -> I'll publish it another npm package so that I can use it/
Just wanted to sync up with you before making a decision here

Btw neat plugin <3 love ittttt

- udpate documentation
- set default format to json
- supported formats are json, fullName, minimal
@thelostone-mc thelostone-mc changed the title chore: replace pretty with format parameter feat: replace pretty with format parameter Jul 2, 2022
Copy link
Owner

@ItsNickBarry ItsNickBarry left a comment

Choose a reason for hiding this comment

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

I wasn't aware of any human readable ABI other than the "minimal" format. Could you post an example?

This seems like a reasonable change, but I don't want to make any breaking changes until next major version, which I"m not ready to release yet. However, it should be easy to keep both the pretty and format options for now, and remove pretty later. See inline comments.

README.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
tasks/export_abi.js Outdated Show resolved Hide resolved
@thelostone-mc
Copy link
Contributor Author

Ah sweet ! Will address the feedback

And yup i came across this different formats just a few hours earlier 😅

This is an example of FormatTypes.fullName

'function transferFrom(address from, address to, uint256 value)',

This is an example of FormatTypes.minimal (which is what pretty option does)

'function transferFrom(address,address,uint256)',

This section covers the 3 abi options in a bit more detail:
https://docs.ethers.io/v5/api/utils/abi/formats/#abi-formats

@thelostone-mc thelostone-mc changed the title feat: replace pretty with format parameter feat: add format parameter to hardhat-abi-exporter Jul 2, 2022
tasks/export_abi.js Outdated Show resolved Hide resolved

if (!config.format) {
conf.format = conf.pretty ? "minimal": "json";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ItsNickBarry let me know if you are good with this approach.
pretty internally just uses format and that way code is less messier in export_abi.js file

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this is the best way. Similar to flat/rename above.


if (!config.format) {
conf.format = conf.pretty ? "minimal": "json";
}
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this is the best way. Similar to flat/rename above.

tasks/export_abi.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ItsNickBarry
Copy link
Owner

Fixed a couple of things. See resolve comments. I'll merge and publish this soon.

@thelostone-mc
Copy link
Contributor Author

Perfect :D Thanks again for the super quick feedback

@ItsNickBarry ItsNickBarry merged commit 90635b7 into ItsNickBarry:master Jul 3, 2022
@ItsNickBarry
Copy link
Owner

Post-merge fix: 71e7dcb

@ItsNickBarry
Copy link
Owner

Thanks for the PR, @thelostone-mc. Released in 2.10.0.

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