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

Cover all remaining statements and expressions in the spec #514

Merged
merged 25 commits into from
Aug 26, 2021

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Aug 16, 2021

What was wrong?

The spec didn't cover all statements and expressions of the language.

How was it fixed?

Added basic coverage of all statements and expressions plus a few extra goodies

@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2021

Codecov Report

Merging #514 (1a5e939) into master (a0821aa) will not change coverage.
The diff coverage is n/a.

❗ Current head 1a5e939 differs from pull request most recent head 9d0a748. Consider uploading reports for the commit 9d0a748 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #514   +/-   ##
=======================================
  Coverage   87.51%   87.51%           
=======================================
  Files          86       86           
  Lines        5550     5550           
=======================================
  Hits         4857     4857           
  Misses        693      693           

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 a0821aa...9d0a748. Read the comment docs.

@cburgdorf cburgdorf force-pushed the christoph/docs/spec-content branch 3 times, most recently from 7ec8375 to ea61b14 Compare August 19, 2021 10:38
@cburgdorf cburgdorf force-pushed the christoph/docs/spec-content branch 2 times, most recently from 6cfed48 to eb09d5d Compare August 23, 2021 09:30
@cburgdorf cburgdorf marked this pull request as ready for review August 24, 2021 13:59
@cburgdorf cburgdorf changed the title More spec stuff Cover all remaining statements and expressions in the spec Aug 24, 2021
@cburgdorf
Copy link
Collaborator Author

Btw, I'm going to add a CI check to ensure our code examples won't go out of date soon but that will be a different PR.

@cburgdorf
Copy link
Collaborator Author

Ok, I ended up adding the CI check to this PR (but happy to move it out on request) and it already found a few code examples that were out of date.

Copy link
Member

@g-r-a-n-t g-r-a-n-t left a comment

Choose a reason for hiding this comment

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

Looks great. I love that we check the snippets now.


return sum

fn some_skip_condition -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

hmm.. looks like the parser is letting func defs without parens through

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah, looks like the parser only disallows missing parens if the function name is immediately followed by a : and hence declares no return type. I filed an issue: #523

@cburgdorf cburgdorf merged commit f1b8f9c into ethereum:master Aug 26, 2021
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