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

chore: add contribution guide and document scripts/vm.py #618

Merged
merged 6 commits into from
Oct 10, 2024

Conversation

zerosnacks
Copy link
Member

@zerosnacks zerosnacks commented Oct 4, 2024

Copied from https://github.com/foundry-rs/foundry/blob/master/CONTRIBUTING.md with some minor modifications to make it specific to forge-std

Documents running vm.py to update src/Vm.sol in more detail, related: #615 (comment) as well as the new --from flag

@zerosnacks zerosnacks requested a review from mds1 October 4, 2024 09:16
@zerosnacks zerosnacks changed the title chore: add contribution guide chore: add contribution guide and document scripts/vm.py Oct 4, 2024
@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 4, 2024

To generate the changes from my PR on Foundry repo I had to manually modify vm.py to fetch the json from my repo+branch.

It would be nice to be able to pass that information to vm.py, something like:

$ ./scripts/vm.py tudmotu vm.mockCalls

And it would fetch the json file from https://raw.githubusercontent.com/{username}/foundry/{branch}/crates/cheatcodes/assets/cheatcodes.json

These params could be optional and default to "foundry-rs" and "master" respectively, to keep backwards-compatibility.

WDYT?

@zerosnacks
Copy link
Member Author

To generate the changes from my PR on Foundry repo I had to manually modify vm.py to fetch the json from my repo+branch.

It would be nice to be able to pass that information to vm.py, something like:

$ ./scripts/vm.py tudmotu vm.mockCalls

And it would fetch the json file from https://raw.githubusercontent.com/{username}/foundry/{branch}/crates/cheatcodes/assets/cheatcodes.json

These params could be optional and default to "foundry-rs" and "master" respectively, to keep backwards-compatibility.

WDYT?

Ah I see, I generally wouldn't be against making this addition

In terms of process it is usually the case that cheatcodes are first merged and then a follow-up PR in forge-std is to be drafted rather than concurrently. Defaulting to the existing case is a good middle ground I feel.

Would you like to contribute this change?

@Tudmotu
Copy link
Contributor

Tudmotu commented Oct 4, 2024

Sure, will open a small PR using argparse and you can decide if you want to include it.

Edit: #619

CONTRIBUTING.md Outdated Show resolved Hide resolved
@zerosnacks zerosnacks requested a review from mds1 October 7, 2024 09:06
If the answer is not there:

- Join the [support Telegram](https://t.me/foundry_support) to get help, or
- Open a [discussion](https://github.com/foundry-rs/foundry/discussions/new) with your question, or
Copy link
Member

Choose a reason for hiding this comment

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

we probably should remove this

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this could still be useful as users generally don't know the distinction between Foundry / forge-std

Or are you in favor of deprecating either / any support portals? We haven't seen a lot of use of the Github discussions but it is a good place to find answers if we clean it up a bit

Copy link
Member Author

Choose a reason for hiding this comment

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

Can be handled in a follow-up if desired cc @mds1

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I mean the discussions.

README.md Outdated Show resolved Hide resolved
@zerosnacks
Copy link
Member Author

Going to merge as it is relatively non-controversial and should help external contributors to run / update

Happy to handle any follow-ups / modifications

@zerosnacks zerosnacks merged commit 4b16934 into master Oct 10, 2024
3 checks passed
@zerosnacks zerosnacks deleted the zerosnacks/add-contribution-guide branch October 10, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

4 participants