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

Rewrite #350

Open
5 tasks
frangio opened this issue Jan 18, 2022 · 9 comments
Open
5 tasks

Rewrite #350

frangio opened this issue Jan 18, 2022 · 9 comments

Comments

@frangio
Copy link
Contributor

frangio commented Jan 18, 2022

solidity-docgen is being rewritten to be simpler and easier to maintain and contribute to. These goals haven't been realized yet mainly because there is pending documentation and the code lacks some explanatory comments, but the point of this rewrite is that there should be a lot less to explain!

The work is mostly done and can be found in the rewrite branch.

This is a breaking release, your existing templates will not work without some changes. The CLI is no longer available (but may be brought back later), a Hardhat plugin is available instead, as well as standalone use as a library.

There are some pending tasks:

  • Finish default templates. Must look good and do a good job of presenting all info and docs in the contracts.
  • Write detailed usage documentation including template customization.
  • Ensure there are explanatory comments in important parts of the source code.
  • Write tutorial to set up together with Vuepress.
  • Add changelog section for new version. Emphasize breaking changes.

A beta can be installed from npm as solidity-docgen@next, but this is experimental and can fail. If you try it out, report any issues.

@frangio frangio pinned this issue Jan 18, 2022
@0xVolosnikov
Copy link
Contributor

This regex doesn't clear correctly lines with multiple spaces at the beginning. Like this:

/**
*  @notice foo
*  @dev baz
*/

may be worth replacing it like that:
.replace(/^[ \t]+/mg, '');

@frangio
Copy link
Contributor Author

frangio commented Jan 26, 2022

@vladyan18 The reason I didn't do that is that indentation can be significant. For example if the contents of the comment contain a Solidity example:

/**
 * This function should be protected against reentrancy:
 * ```
 * function foo() nonReentrant {
 *     _bar();
 * }
 * ```
 */

The indentation inside the function would be removed by the regex you suggested.

An alternative would be to find the shortest common whitespace prefix across all lines. In my example it would be a single space, in your example it would be two spaces.

But I also think the current behavior is quite reasonable. 🙂 Do you think it's worth changing this?

@0xVolosnikov
Copy link
Contributor

@frangio Indeed, your example is correct. Another option would be to use something like .replace(/^[ \t]?@/mg, '@');, but it can make a mess.

So it's probably best to really leave it as it is.

@0xVolosnikov
Copy link
Contributor

@frangio Also, I had a problem when I used inheritdoc with public state variables.

This check forbids it.

@frangio
Copy link
Contributor Author

frangio commented Jan 26, 2022

Thanks. I have a fix for that I'll publish later.

@frangio
Copy link
Contributor Author

frangio commented Jan 28, 2022

@vladyan18 Just published the full suite of templates in a new beta. If you're testing this, I'd love to know what you think.

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Mar 18, 2022

Hi @frangio! Great work with this refactor. I upgraded to the next version but I got stuck trying to understand how the new version works. A simple migration guide would be helpful. Specifically the issue is that I don't know how to port my current template:

https://github.com/hifi-finance/hifi/blob/dc748c7ff66d652a00463dc57ea7c0cc12f0b289/templates/contract.hbs

To the newer format of v0.6. Should I provide one Handlebars template for each type of item? if yes, how can I do this? Do I have to work with the pages setting in Hardhat?

@frangio
Copy link
Contributor Author

frangio commented Mar 21, 2022

Hi @PaulRBerg, glad you've tried it. It's taking me longer than I wanted to finish this new version and work on the documentation including a migration guide, but it's pretty stable by now. To customize the templates you need to set the templates parameter in the Hardhat config to name a directory of templates to override the default ones (can be just a subset and the rest use the default). As a starting point you can just copy the ones you need to customize from themes/markdown to your templates directory.

Yes, each type of item has its own template, this is because items are not necessarily inside a contract now (e.g. free functions).

I took a look at your repo and migrated it to the new version to see what that was like. Some of the template field names are kind of different, I'm wondering if I should add a compatibility layer to ease the migration.

Please take a look at frangio/hifi@abea0bc and let me know if you have any feedback about the config and the output.

@PaulRBerg
Copy link
Contributor

Whoah, thanks so much for taking the time to migrate our repo for us. This is super helpful - it would have taken me ages to achieve the same result.

Feedback:

  • Looks like the bug whereby the returned value types were generated incorrectly is now gone! In the past solidity-docgen used to generate mistaken types for the returned values of functions (e.g. address instead of uint256).
  • It isn't super obvious that returning "undefined" in the pages function in the Hardhat config means there's no page generated for that item. What if instead of that one could return "none" (as a string)?
  • It would be nice if the behavior that you implemented via the printParams function would be available by default, without the need for a custom helpers.js file.
  • The one-contract-one-page organization mode should be common enough to warrant a default option in solidity-docgen. I made a PR for this: feat: add "contracts" default option for "pages" #365

And finally:

Some of the template field names are kind of different, I'm wondering if I should add a compatibility layer to ease the migration.

Yes, a compatibility layer would be nice. I actually forked the old template from Uniswap, so it's likely that many solidity-docgen users are relying on the previous fields.

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

No branches or pull requests

3 participants