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

feat[lang]!: make @external modifier optional in .vyi files #4178

Merged

Conversation

cyberthirst
Copy link
Collaborator

@cyberthirst cyberthirst commented Jul 24, 2024

What I did

How to verify it

  • added some tests

Commit Message

make `@external` visibility in `.vyi` files optional.

additionally, fix a panic when functions in an interface have the wrong
visibility

edit: linked wrong issue

@cyberthirst cyberthirst added this to the v0.4.1 milestone Jul 26, 2024
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

thanks!

@charles-cooper charles-cooper changed the title feat[lang]!: make external visibility in vyi files optional feat[lang]!: make @external modifier optional in .vyi files Aug 5, 2024
@charles-cooper charles-cooper enabled auto-merge (squash) August 5, 2024 16:23
@charles-cooper charles-cooper merged commit c0cf436 into vyperlang:master Aug 5, 2024
155 checks passed
d for d in funcdef.decorator_list if d.id in FunctionVisibility.values()
)
raise FunctionDeclarationException(
"Interface functions can only be marked as `@external`", nonexternal
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the error message can be confusing since it could imply that payable is disallowed (which is not). @cyberthirst how about:

Interface functions' visibility can only be marked as `@external`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm, we've improved the reporting a bit, so the message refers to the corresponding source construct

@internal
@payable
def bar():
    ...

would yield:

vyper.exceptions.FunctionDeclarationException: Interface functions can only be marked as `@external`

  contract "tests/custom/i.vyi:1", function "bar", line 1:1 
  ---> 1 @internal
  --------^
       2 @payable

would you say that it's still confusing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally think error messages should be as precise as possible and thus I still think we should mention somehow that it's a visibility decorator.

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.

Panic on expected interface method implemented as internal.
3 participants