⚠️ No Changeset found

Latest commit: 767fb0e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

This is awesome, thank you! Since this is still a WIP, I'm going to convert this PR into a draft. Feel free to convert it back once you're ready for review.

I've just added the validation script that goes through the output and checks if there are any missing Natspec comments.
The task is currently ran on compilation & can be configured to throw. (no custom GH action will be required)

Next:

1. Validate that the Dodoc md format is okay with Optimism.

• Feedback required.
  --> If not ok -> I can edit the dodoc source (fork / PR)

2. Add Config to the validate-output task
   --> error = throw or warn.
   --> checks = list of enabled checks.
   --> include; exclude; runOnCompile;

3. Beautify the console output.

4. Make a separate package (hardhat plugin) with the validate-output task. (optional)

Codecov Report

Merging #1937 (767fb0e) into develop (a5001aa) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #1937   +/-   ##
========================================
  Coverage    74.42%   74.42%           
========================================
  Files           79       79           
  Lines         2545     2545           
  Branches       397      397           
========================================
  Hits          1894     1894           
  Misses         651      651           

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5001aa...767fb0e. Read the comment docs.

Update:

• Added the config options for the output-checker.
• Added one extra check = "Compilation Warnings" in build.
• Refactored based on Elena's feedback :)

Next:

• Get Feedback on approach & output (both md and logs).
• Let me know if there are any other requirnments (How would the docs be deployed live ?).
• Thorough Testing.
• Move code to a npm package :)

Update:

• Added the config options for the output-checker.
• Added one extra check = "Compilation Warnings" in build.
• Refactored based on Elena's feedback :)

Next:

• Get Feedback on approach & output (both md and logs).
• Let me know if there are any other requirnments (How would the docs be deployed live ?).
• Thorough Testing.
• Move code to a npm package :)

Update:

• Around 50-60 "missing comments" are found by the new plugin.
• Plugin is published in npm - hardhat-output-validator
• Awaiting Feedback & Additional Work :)

Can you please update the docs to docs/Initializable.md which appears to have changed.

Another thing for the wish list is to also detect incorrect or missing function parameters or return values. I am certain this was a feature of the solidity complier before although not sure why we're not getting these reports now.

There is also one failure in CI on https://github.com/ethereum-optimism/optimism/runs/4616712161?check_suite_focus=true if you can fix pls.

Can you please update the docs to docs/Initializable.md which appears to have changed.

Another thing for the wish list is to also detect incorrect or missing function parameters or return values. I am certain this was a feature of the solidity complier before although not sure why we're not getting these reports now.

👍 Updated.
🥇 Added support for @return & @param !
I as well have a memory of these errors of the compiler. However I believe the "DocstringParsingError" appears only when you misspell a @param name and not when it's missing.

Nice but missing @param comments are now being inherited from external libraries, e.g.

Comments Error: Function: (transferOwnership) is missing @param comment
   @ Lib_AddressManager
   --> contracts/libraries/resolver/Lib_AddressManager.sol

Nice but missing @param comments are now being inherited from external libraries, e.g.

Comments Error: Function: (transferOwnership) is missing @param comment
   @ Lib_AddressManager
   --> contracts/libraries/resolver/Lib_AddressManager.sol

Pff this proved to be tricky.

Should be working now :)

Yep working now thanks. Checking output one by one, meanwhile I found these to be false positives

Comments Error: Function: (deleteElementsAfterInclusive), param: (_globalMetadata) is missing @param comment
   @ IChainStorageContainer
   --> contracts/L1/rollup/IChainStorageContainer.sol`

Comments Error: Function: (push), param: (_globalMetadata) is missing @param comment
   @ IChainStorageContainer
   --> contracts/L1/rollup/IChainStorageContainer.sol

Maybe there is a problem with overloads?

Yep working now thanks. Checking output one by one, meanwhile I found these to be false positives

Comments Error: Function: (deleteElementsAfterInclusive), param: (_globalMetadata) is missing @param comment
   @ IChainStorageContainer
   --> contracts/L1/rollup/IChainStorageContainer.sol`

Comments Error: Function: (push), param: (_globalMetadata) is missing @param comment
   @ IChainStorageContainer
   --> contracts/L1/rollup/IChainStorageContainer.sol

Maybe there is a problem with overloads?

Fixed. Good catch ! :)
Dodoc has the same issue - overloads don't get their docs generated. I've created a PR for this as well :)