Ensure constant getters show in docs #4649
Conversation
|
| ); | ||
| }; | ||
|
|
||
| module.exports.returns2 = function ({ item }) { |
There was a problem hiding this comment.
Why not returnsExtended or similar? I don't think returns2 is really expressive
There was a problem hiding this comment.
I prefer the short version, if you feel strongly about it we could use returnsExtended.
There was a problem hiding this comment.
Let's keep returns2, don't want to block this
docs/templates/properties.js
Outdated
| module.exports.functions = function ({ item }) { | ||
| return [...findAll(['VariableDeclaration', 'FunctionDefinition'], item)].filter(f => | ||
| f.nodeType === 'VariableDeclaration' ? f.visibility === 'public' : f.visibility !== 'private', | ||
| ); | ||
| }; |
There was a problem hiding this comment.
IIUC this should be updated in solidity-docgen, is it?
There was a problem hiding this comment.
Yes, but it's more complicated because of backwards compatibility. functions currently returns FunctionDefinition nodes and not VariableDeclaration nodes, so it might break some templates.
|
I moved getters to the end of the list of functions because they were showing up before the constructor. |
|
Updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
| return [ | ||
| ...[...findAll('FunctionDefinition', item)].filter(f => f.visibility !== 'private'), | ||
| ...[...findAll('VariableDeclaration', item)].filter(f => f.visibility === 'public'), | ||
| ]; |
There was a problem hiding this comment.
I think this works:
| return [ | |
| ...[...findAll('FunctionDefinition', item)].filter(f => f.visibility !== 'private'), | |
| ...[...findAll('VariableDeclaration', item)].filter(f => f.visibility === 'public'), | |
| ]; | |
| return [ | |
| ...findAll('FunctionDefinition', item).filter(f => f.visibility !== 'private'), | |
| ...findAll('VariableDeclaration', item).filter(f => f.visibility === 'public'), | |
| ]; |
There was a problem hiding this comment.
Also, we can keep the constructor getters at first
| return [ | |
| ...[...findAll('FunctionDefinition', item)].filter(f => f.visibility !== 'private'), | |
| ...[...findAll('VariableDeclaration', item)].filter(f => f.visibility === 'public'), | |
| ]; | |
| const functionDefs = findAll('FunctionDefinition', item) | |
| const constructor = functionDefs.find(f => f.kind == 'constructor'); | |
| return [ | |
| constructor, | |
| ...findAll('VariableDeclaration', item).filter(f => f.visibility === 'public'), | |
| ...functionDefs.filter(f => f.visibility !== 'private') | |
| ]; |
For cases like AccessManager and AccessControl I think it's valuable to have the ADMIN_ROLE/DEFAULT_ADMIN_ROLE definitions at first.
It's true that ADMIN_ROLE and PUBLIC_ROLE don't have NatSpec anyways, so I wouldn't have problems keeping the current code.
There was a problem hiding this comment.
I think this works
It doesn't because findAll is a generator function, it doesn't return an array.
| ); | ||
| }; | ||
|
|
||
| module.exports.returns2 = function ({ item }) { |
There was a problem hiding this comment.
Let's keep returns2, don't want to block this
(cherry picked from commit 39400b7)
The main example of what was missing can be seen in
UPGRADE_INTERFACE_VERSIONinUUPSUpgradeable.