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: adding Enum #2874

Merged
merged 27 commits into from
May 31, 2022
Merged

feat: adding Enum #2874

merged 27 commits into from
May 31, 2022

Conversation

pandadefi
Copy link
Contributor

@pandadefi pandadefi commented May 25, 2022

What I did

Adding enums to vyper

fixes: #2319

How I did it

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

image

@pandadefi pandadefi marked this pull request as draft May 25, 2022 16:51
@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 25, 2022

This pull request introduces 6 alerts when merging e5487e1 into 42a372e - view on LGTM.com

new alerts:

  • 6 for Unused import

return Enum(abi["name"], abi["inputs"])

@classmethod
def from_EnumDef(cls, base_node: vy_ast.EventDef) -> "Enum":
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def from_EnumDef(cls, base_node: vy_ast.EventDef) -> "Enum":
def from_EnumDef(cls, base_node: vy_ast.EnumDef) -> "Enum":


Arguments
---------
base_node : EventDef
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
base_node : EventDef
base_node : EnumDef

@classmethod
def from_EnumDef(cls, base_node: vy_ast.EventDef) -> "Enum":
"""
Generate an `Event` object from a Vyper ast node.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Generate an `Event` object from a Vyper ast node.
Generate an `Enum` object from a Vyper ast node.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 26, 2022

This pull request introduces 6 alerts when merging 612d2bd into 67bb98a - view on LGTM.com

new alerts:

  • 6 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 26, 2022

This pull request introduces 5 alerts when merging 4f07fa6 into 67bb98a - view on LGTM.com

new alerts:

  • 5 for Unused import

@codecov-commenter
Copy link

codecov-commenter commented May 26, 2022

Codecov Report

Merging #2874 (277fdb7) into master (457a8b6) will increase coverage by 0.26%.
The diff coverage is 85.48%.

@@            Coverage Diff             @@
##           master    #2874      +/-   ##
==========================================
+ Coverage   87.69%   87.95%   +0.26%     
==========================================
  Files          94       95       +1     
  Lines       10196    10355     +159     
  Branches     2504     2498       -6     
==========================================
+ Hits         8941     9108     +167     
- Misses        790      792       +2     
+ Partials      465      455      -10     
Impacted Files Coverage Δ
vyper/ast/pre_parser.py 96.82% <ø> (ø)
vyper/semantics/types/bases.py 84.34% <0.00%> (-0.36%) ⬇️
vyper/codegen/types/convert.py 94.73% <75.00%> (+0.61%) ⬆️
vyper/semantics/types/user/enum.py 80.70% <80.70%> (ø)
vyper/codegen/types/types.py 88.34% <86.20%> (-0.09%) ⬇️
vyper/codegen/expr.py 79.43% <87.50%> (-1.30%) ⬇️
vyper/ast/nodes.py 94.08% <100.00%> (-0.16%) ⬇️
vyper/ast/signatures/function_signature.py 89.39% <100.00%> (+1.33%) ⬆️
vyper/ast/signatures/interface.py 81.48% <100.00%> (ø)
vyper/codegen/global_context.py 74.78% <100.00%> (+0.87%) ⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 457a8b6...277fdb7. Read the comment docs.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 26, 2022

This pull request introduces 10 alerts when merging af12e16 into 67bb98a - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 3 for Wrong number of arguments in a call
  • 2 for Wrong name for an argument in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 26, 2022

This pull request introduces 10 alerts when merging de0717a into 67bb98a - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 3 for Wrong number of arguments in a call
  • 2 for Wrong name for an argument in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 26, 2022

This pull request introduces 10 alerts when merging 843b19d into 67bb98a - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 3 for Wrong number of arguments in a call
  • 2 for Wrong name for an argument in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 27, 2022

This pull request introduces 9 alerts when merging 61cedc7 into 67bb98a - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 2 for Wrong name for an argument in a call
  • 2 for Wrong number of arguments in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 27, 2022

This pull request introduces 9 alerts when merging f2dd34a into 67bb98a - view on LGTM.com

new alerts:

  • 6 for Unused import
  • 2 for Wrong name for an argument in a call
  • 1 for Wrong number of arguments in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 28, 2022

This pull request introduces 8 alerts and fixes 5 when merging 4fb2ede into f5051e1 - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 2 for Wrong name for an argument in a call
  • 1 for Syntax error

fixed alerts:

  • 4 for `__eq__` not overridden when adding attributes
  • 1 for 'import *' may pollute namespace

@charles-cooper
Copy link
Member

whoops, re-requested review but maybe needs some more tests. still in draft mode

@lgtm-com
Copy link
Contributor

lgtm-com bot commented May 28, 2022

This pull request introduces 2 alerts when merging 2593c62 into f5051e1 - view on LGTM.com

new alerts:

  • 2 for Wrong name for an argument in a call

@charles-cooper charles-cooper marked this pull request as ready for review May 30, 2022 17:08
@charles-cooper
Copy link
Member

lgtm, but i contributed some work to this and so we should get a review from @fubuloubu as well.

@charles-cooper
Copy link
Member

charles-cooper commented May 30, 2022

one q i have is whether we should introduce any conventions around naming of enum members. options:

enum Action:
    buy
    sell
enum Action:
    Buy
    Sell
enum Action:
    BUY
    SELL

@fubuloubu
Copy link
Member

one q i have is whether we should introduce any conventions around naming of enum members.

Typically this is the way enums are usually specified

enum Action:
    BUY
    SELL

Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Some nits on naming

docs/types.rst Outdated Show resolved Hide resolved
docs/types.rst Outdated Show resolved Hide resolved
charles-cooper and others added 2 commits May 30, 2022 18:14
Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>
@charles-cooper charles-cooper enabled auto-merge (squash) May 31, 2022 01:21
@charles-cooper charles-cooper merged commit a8a23b8 into vyperlang:master May 31, 2022
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.

VIP: Add support for Enumerations
5 participants