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 preparation commit command pnpm adorno #23

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

3esmit
Copy link

@3esmit 3esmit commented Feb 26, 2024

Description

Fixes #19

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • Added natspec comments?
  • Ran forge snapshot?
  • Ran pnpm gas-report?
  • Ran pnpm lint?
  • Ran forge test?
  • Ran pnpm verify?

@3esmit 3esmit requested a review from 0x-r4bbit February 26, 2024 15:15
@3esmit 3esmit self-assigned this Feb 26, 2024
@3esmit 3esmit force-pushed the prepare-commit-cmd branch from 7a2d305 to d76b33f Compare February 26, 2024 17:05
@3esmit 3esmit changed the title chore: add preparation commit command pnpm prepare chore: add preparation commit command pnpm adorno Feb 26, 2024
- [ ] Ran `pnpm gas-report`?
- [ ] Ran `pnpm lint`?
- [ ] Ran `forge test`?
- [ ] Ran `pnpm adorno`?
Copy link
Collaborator

Choose a reason for hiding this comment

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

There could be an argument that removing forge test here is not ideal, but on the other hand, this will then latest fail during CI when there's failing tests. So I think it's fine, but I still wanted to point it out.

Copy link
Author

Choose a reason for hiding this comment

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

forge test is implicit on forge snapshot and pnpm gas-report.

@0x-r4bbit
Copy link
Collaborator

@3esmit FYI: if you still want to rename this commit message, you can still do it and then merge (or merge right away)

@3esmit
Copy link
Author

3esmit commented Feb 29, 2024

I cant merge this, button is grayed out.

@0x-r4bbit
Copy link
Collaborator

@3esmit the reason it's grayed out is because your commits are signed.
You might want to set up GPG for this: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@0x-r4bbit
Copy link
Collaborator

Also, tip for the future:

We probably don't want to merge main into developing branches.
The developing branches should always be rebased on top of main, and then get merged into main.

That avoids us introducing these merge commits: e1f6eb1

@0x-r4bbit 0x-r4bbit force-pushed the prepare-commit-cmd branch from e1f6eb1 to 3df4d72 Compare March 1, 2024 09:30
@0x-r4bbit
Copy link
Collaborator

@3esmit FYI: I've rebased your adorno commit on top of latest main so this can be merged fast-forward.

@0x-r4bbit 0x-r4bbit merged commit 96e97ba into main Mar 1, 2024
7 checks passed
@0x-r4bbit 0x-r4bbit deleted the prepare-commit-cmd branch March 1, 2024 09:35
@3esmit
Copy link
Author

3esmit commented Mar 1, 2024

I don't mind, just want to understand the reason behind putting adorno as last command in package.json instead of how I left?

@0x-r4bbit
Copy link
Collaborator

@3esmit Ah, no particular reason. There was a merge conflict as there was the release command and the adorno command both modifying the same line in package.json.

I just resolved the conflict without paying much attention to the order of the commands.
Feel free to change this.

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

Successfully merging this pull request may close these issues.

Use a single command to do prepare for PR
2 participants