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

Missing entries in devdoc / userdoc [natspec] #1141

Closed
4 of 6 tasks
axic opened this issue Oct 4, 2016 · 19 comments
Closed
4 of 6 tasks

Missing entries in devdoc / userdoc [natspec] #1141

axic opened this issue Oct 4, 2016 · 19 comments

Comments

@axic
Copy link
Member

axic commented Oct 4, 2016

  /// @notice bb
  /// @dev aa
  /// @param a first
  /// @param c third
  /// @param b second
  /// @return test
  /// @author bobby
  function a(uint a, uint c, uint b) {
    throw;
  }

Developer Documentation
{
   "methods" : {
      "a(uint256,uint256,uint256)" : {
         "author" : "bobby",
         "details" : "aa",
         "params" : {
            "a" : "first",
            "b" : "second",
            "c" : "third"
         },
         "return" : "test"
      }
   }
}

User Documentation
{
   "methods" : {
      "a(uint256,uint256,uint256)" : {
         "notice" : "bb"
      }
   }
}

Natspec:

@IstoraMandiri
Copy link

IstoraMandiri commented Oct 27, 2016

  • How about modifiers, struct, event, variable definitions?
  • It appears that only @name and @author work for the contract definition. Would be nice to include @dev and @notice.
  • should docs also be outputted for inherited methods? otherwise, doc generators will need to manually parse the AST to determine inherited methods
  • I noticed that the method params are getting added to devdoc rather than userdoc, contradicting the docs

@axic axic changed the title Missing entries in devdoc / userdoc Missing entries in devdoc / userdoc [natspec] Nov 16, 2016
@axic
Copy link
Member Author

axic commented Nov 16, 2016

@hitchcott would you be interested in helping to design a better version of the natspec? I'm thinking a combined user/devdoc.

@IstoraMandiri
Copy link

Sounds good @axic, I am actively working on doxity and there's good opportunity to explore what is needed. What's the best way for me to help with this - should I draft an EIP?

@axic
Copy link
Member Author

axic commented Nov 17, 2016

@hitchcott we could have a chat on gitter with anyone interested in this topic.

Also please check https://github.com/ethereum/wiki/wiki/Ethereum-Natural-Specification-Format first as that defines the userdoc.

@ethers
Copy link
Member

ethers commented Nov 20, 2016

@axic @hitchcott After your chat on gitter, could an EIP be written to make it clear? (The wikis are mostly old.)

One part I'm wary of is if returns are going to need to be named, like:
function foo() returns (uint r)
Since r is actually a variable and may be unused, I think it leads to issues like
#718 (comment)

For these named returns to be used wider, I think Solidity should be much stricter to prevent misuse: example #1401

@chriseth
Copy link
Contributor

Note that the only purpose of @notice is to display a message to the user if they invoke a function. This does apply to the fallback function, it may apply to the constructor, but not anything else.

This means that comments applying to the contract itself should probably automatically go to the @dev scope.

The @notice docs should be inherited.

Method parameters should probably go to both the dev docs and the user docs.

@ethers
Copy link
Member

ethers commented Dec 2, 2016

/// @notice (balanceInmGAV / 1000).fixed(0,3) GAV is the total funds available to who.address().
/// @param who The address of the person whose balance we check
/// @return The balance of the user provided as argument
function balance(address who) constant returns (uint256 balanceInmGAV) {
return 100;
}

In the example above, it uses an explicit return 100. To convey that the 100 is balanceInmGAV, is an unused variable balanceInmGAV needed?

Can we do something like this instead, where we move balanceInmGAV to be strictly a comment?
/// @notice (balanceInmGAV / 1000).fixed(0,3) GAV is the total funds available to who.address().
/// @param who The address of the person whose balance we check
/// @return balanceInmGAV The balance of the user provided as argument
function balance(address who) constant returns (uint256) {
return 100;
}

I don't understand this example much, but my point is comments should remain as comments and should not require introducing an unused variable in code.

@chriseth
Copy link
Contributor

chriseth commented Dec 2, 2016

Sometimes, you need the ability to refer to individual elements of the return tuple. The example is bad, though, I agree.

@ethers
Copy link
Member

ethers commented Dec 9, 2016

For refering to tuple elements, just an idea:
/// @return (balance, nonce, counter)
/// @@balance The balance of the user provided as argument
/// @@nonce The nonce of the ...
/// @@counter The counter of the ...

function foo() constant returns (uint256, uint256, uint256)

The comment (balance, nonce, counter) is matched against the tuple (uint256, uint256, uint256).

With tuples, an explicit return might even be more important for clarity:
return (balance, nonce, counter) instead of example:

function foo() constant returns (uint256 balance, uint256 nonce, uint256 counter) {
  // some code
  nonce = 1;  // nonce is computed first
  // some code
  counter = 2; // then counter
  // some code
  balance = someMap[msg.sender];
}

@chriseth
Copy link
Contributor

chriseth commented Dec 9, 2016

Actually I think @return should be very similar to @param. How does javadoc/doxygen handle unnamed parameters? I think we can be a bit flexible here:
If there is an identifier after @return, try to look it up in the list of return values. If it is there, this is the documentation about that value. If it is of the form [i], it is the documentation of the ith return value. Otherwise, it is a string describing the full return tuple.

@frangio
Copy link
Contributor

frangio commented Aug 28, 2017

What's the status of this? There's an annoying problem with functions which have unused arguments because they're meant to be overriden: if you remove the argument name so as to silence the warning, it's impossible to add @param documentation for it.

@tymat
Copy link

tymat commented Sep 28, 2017

Would also like to know the status of this particularly:

  • documentation for internal and private functions
  • multiple return values

@feuGeneA
Copy link

Please add these items to the checklist in the original message:

  • Support for @dev tag on a contract
  • Support for events

Also, I think you can safely check off the item "parameter names." The devdoc output from v0.4.24 is showing parameter names:

            "publicMethod(int256,address,bool)": {
                "details": "publicMethod @dev",
                "params": {
                    "a": "publicMethod @param 2",
                    "b": "publicMethod @param 3",
                    "p": "publicMethod @param"
                },
                "return": "publicMethod @return"
            }

And, with the merging of #4542 , it seems that you can also check off "constructor."


Also, @chriseth , you mentioned above that @notice does not apply to the contract definition, but the wiki explicitly says it does. Should the wiki be updated, or were you mistaken in that comment?

@chriseth
Copy link
Contributor

The wiki is unmaintained.

@corydickson
Copy link
Contributor

Any updates on multiple return values? Leaning towards implementing an approach where @return functions just like @param as per @chriseth suggestion. The field would be of the type (string|object)? depending on the number of identifiers after the first @return, where the ith item in the object has the description for the ith return value in the tuple.

Is there any more pushback for supporting named return values as well? That way we can use them as keys instead of relying on the order of the tuple, i.e. making it easier to deal with verifying nested tuple types.

@axic
Copy link
Member Author

axic commented Mar 17, 2021

I think we have specific issues now for everything, as well as the natspec project: https://github.com/ethereum/solidity/projects/42

Is there anything else missing from there? Can we close this issue?

@feuGeneA
Copy link

I think we have specific issues now for everything, as well as the natspec project: https://github.com/ethereum/solidity/projects/42

Is there anything else missing from there? Can we close this issue?

In comments above, I had asked for these things to be added to this issue:

  • Support for @dev tag on a contract
  • Support for events

I'd still like to see those happen (if they haven't already) but I really don't care whether they're tracked in this issue or elsewhere.

@axic
Copy link
Member Author

axic commented Mar 17, 2021

@feuGeneA Could you please create new separate issues for them? We have found that individual issues work better nowadays.

@axic
Copy link
Member Author

axic commented Mar 24, 2021

As an update, I think this issue can now be closed as parts are tracked elsewhere.

@axic axic closed this as completed Mar 24, 2021
@axic axic mentioned this issue Mar 24, 2021
6 tasks
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

8 participants