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

remove/useless-label #1298

Closed
wants to merge 3 commits into from
Closed

Conversation

decanus
Copy link
Contributor

@decanus decanus commented Jan 24, 2020

removes useless labels, change the return label to be compliant with ethereum/solidity#7534

@decanus decanus requested a review from mhluongo January 24, 2020 03:41
@mhluongo mhluongo requested a review from ngrinkevich January 24, 2020 03:50
Copy link
Member

@mhluongo mhluongo left a comment

Choose a reason for hiding this comment

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

👍, leaving it to @ngrinkevich for another review and merge.

Keep em coming @decanus 😉

@decanus
Copy link
Contributor Author

decanus commented Jan 24, 2020

@mhluongo only got around to fixing it here, should get around to doing it everywhere else.

@Shadowfiend
Copy link
Contributor

The labels aren't useless, they're how the struct gets assembled on the Go side, which is why the Go build failed in this case. As well, pulling names out of code and adding them to the docs isn't something we're likely to be doing. If a name is in the docs, it should be in the code unless there's no way to specify it there. Code is best considered the ultimate source of truth in these matters.

With that said, thanks for adding the multiple @return docs here and pointing out the relevant Solidity change; I hadn't seen that one come through -- opening a separate PR that updates the multi-returns I noticed and puts proper docs on 'em while we're in there.

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