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

fix(repo): commit contract documentation on branch #13788

Merged
merged 2 commits into from
May 21, 2023
Merged

Conversation

dionysuzx
Copy link
Collaborator

@dionysuzx dionysuzx commented May 20, 2023

this PR has been tested.

why

I am removing the concept of "opening contract documentation in another PR". this job was broken, and i could have fixed it, but i am proposing this previous approach for three reasons:

  1. it is better to commit documentation with protocol changes in the same PR. this means our users know the latest documentation in lock step.
  2. it saves an additional needed PR review
  3. it makes us more aware of always committing good documentation with any protocol changes

the tradeoff is protocol PR's will have *.md files in them, making the diff larger to review. but I think this tradeoff is worth the advantages.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this looks like a good pull request. However, there are a few things to note:

  • The post-merge section has been removed completely. It would be useful to know why it was removed and what the implications are.
  • The protocol - Generate contract documentation step has been replaced with protocol - Commit generated documentation. It would be useful to know why this change was made and how it affects the workflow.
  • The name of the step has changed from Create pull request to protocol - Commit generated documentation. It would be useful to know why this change was made and how it affects the workflow.
  • There is no mention of what is being committed in the commit-message field. It would be useful to have a more descriptive commit message.
  • There is no mention of what the purpose of the new branch is in the branch field. It would be useful to have a more descriptive branch name.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request appears to be making changes to the .github/workflows/protocol.yml file and the packages/protocol/contracts/L1/TaikoL1.sol file.

The changes in the .github/workflows/protocol.yml file look to be removing the post-merge job and replacing it with a new job that runs the command to update the documentation and commit it. The naming of this job is a bit confusing, as it is not clear what the job is doing. I suggest changing the name of this job to something more descriptive, such as "Generate and Commit Protocol Documentation" or something similar.

In the packages/protocol/contracts/L1/TaikoL1.sol file, there is a dummy test that has been added. It looks like this test was added for testing purposes, but should be removed before merging this pull request, as it doesn't appear to add any value to the codebase.

Overall, these changes look good and will help to update the protocol documentation. I recommend making a few minor changes before merging, such as renaming the new job and removing the dummy test from TaikoL1.sol.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this Pull Request looks good. Here are some points to consider:

  • The code changes in the first patch are removing some unnecessary lines and updating the commit message. This looks fine.
  • The second patch is a dummy test and can be removed.
  • It looks like some of the lines in the first patch are being repeated in the second patch. This should be cleaned up.
  • The commit message for the first patch should be updated to include more detail about what changes are being made.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, it looks like the code changes are making improvements to the protocol documentation.

The first patch is removing unnecessary code from the workflow and adding a step to commit the contract documentation. The second patch is making some minor changes to the protocol.yml file. The third patch is making some minor changes to the TaikoL1.sol file and adding a line for a variable.

I suggest making sure that all variables have unique names throughout the codebase, as it will help avoid confusion and potential bugs in the future. Additionally, I suggest making sure that all comments are up to date with the latest changes, as this will help other developers understand what is going on in the codebase.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this pull request looks good. Here are some points I would like to highlight:

  • The protocol - Commit contract documentation step in the post-merge job should be updated to use the commit_message parameter instead of hardcoding the commit message.
  • The protocol - Test Coverage step is commented out. If it is not necessary, it should be removed.
  • The file pattern for the protocol - Commit contract documentation step is missing. It should be added to ensure that only the relevant files are committed.
  • There are several typos in the code. These should be fixed.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this Pull Request looks good. I have the following comments:

  • The code changes seem to be well structured and easy to read.
  • It looks like some redundant code was removed in Patch 1/5 and 2/5.
  • The commit message for Patch 2/5 should be more descriptive.
  • There a few typos in repeating variables such as CHANGELOG.md, CONTRIBUTING.md, etc. These should be fixed.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, the changes look good. I have some comments and feedback:

  • The name and run fields for the protocol - Unit Tests job should be uncommented.
  • The name and run fields for the protocol - Test Coverage job should be uncommented.
  • The name and run fields for the protocol - Upload coverage to Codecov job should be uncommented.
  • The name and run fields for the protocol - Run snapshot (foundry) job should be uncommented.
  • The name and run fields for the protocol - Commit contract documentation job should be added.
  • There are some typos in repeating variables, such as CHANGELOG.md, CONTRIBUTING.md, and Dockerfile. These should be fixed.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this pull request looks good. Here are some comments:

  • It looks like the unit tests have been commented out in the protocol workflow. This should be addressed.
  • It appears that the test coverage and snapshot run jobs have been commented out as well. It would be good to include these in the workflow.
  • The indentation of the code could be improved in some places, for better readability.
  • There is an extra line of code in the TaikoL1.sol file that should be removed.
  • The commit message could be improved to include more information about what this pull request does and why it was made.

@dionysuzx dionysuzx marked this pull request as ready for review May 21, 2023 00:20
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this pull request looks good. It looks like it is introducing a workflow to commit contract documentation on a branch.

Some suggestions:

  • Add a step to validate the contract documentation before committing.
  • Consider adding a step to check if changes have been made before committing.
  • Make sure that the commit message is descriptive and follows the correct formatting conventions.
  • Check the file pattern used for committing the documentation for typos.

@dionysuzx dionysuzx enabled auto-merge May 21, 2023 00:23
@dionysuzx dionysuzx requested review from cyberhorsey, dantaik, davidtaikocha and adaki2004 and removed request for dantaik and davidtaikocha May 21, 2023 00:23
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Overall, this looks like a good change.

The changes made to the protocol.yml file look good. The code is well commented and easy to read.

The addition of the git-auto-commit-action is a nice touch as it will ensure that the documentation is updated with each commit. I suggest adding a comment in the code explaining why this action was added.

It would also be beneficial to add some tests for the documentation update action to make sure that it is working as expected.

@dantaik dantaik removed the request for review from cyberhorsey May 21, 2023 06:18
@dantaik dantaik requested review from dantaik and removed request for adaki2004 May 21, 2023 06:18
@dionysuzx dionysuzx added this pull request to the merge queue May 21, 2023
Merged via the queue into main with commit 2846e75 May 21, 2023
@dionysuzx dionysuzx deleted the fix-contract-gen branch May 21, 2023 06:19
@github-actions github-actions bot mentioned this pull request May 21, 2023
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.

3 participants