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

Clarify visibility of function types vs. contract functions #2623

Closed
wants to merge 1 commit into from
Closed

Clarify visibility of function types vs. contract functions #2623

wants to merge 1 commit into from

Conversation

EricR
Copy link

@EricR EricR commented Jul 23, 2017

Closed #2617

@@ -337,7 +337,8 @@ function type should not return anything, the whole ``returns (<return types>)``
part has to be omitted.

By default, function types are internal, so the ``internal`` keyword can be
omitted.
omitted. Note that this is not the case for functions defined on a contract,
Copy link
Member

@axic axic Jul 23, 2017

Choose a reason for hiding this comment

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

I do not think this is the right way to explain. If you look up a few lines, that lists what is valid for a function type. This would weirdly suggest public is also valid a function type.

Having read the entire section again I think the intro should be clarified, otherwise this feels like only a workaround.

Conversion of functions to function types and the subtleties (with conversion examples) may help clear up the confusion.

Copy link
Author

@EricR EricR Jul 23, 2017

Choose a reason for hiding this comment

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

Hmmm... I see what you mean. Confusion seems to root from the overlap between internal/external function types, and public/private/internal/external contract functions. I agree that explaining the differences first, and providing some examples of conversions between the two, would make this clarification later on a lot clearer. As-is it can cause more confusion, which is obviously not what I intended to do here.

Copy link
Author

Choose a reason for hiding this comment

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

So I've reread this part of the documentation again, and I agree this isn't the right way to explain this at all. I'm going to close this PR.

@EricR EricR closed this Jul 24, 2017
@EricR EricR deleted the patch-1 branch July 24, 2017 00:01
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.

2 participants