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

receive and fallback support #244

Merged
merged 4 commits into from
Feb 29, 2020
Merged

receive and fallback support #244

merged 4 commits into from
Feb 29, 2020

Conversation

Janther
Copy link
Contributor

@Janther Janther commented Feb 28, 2020

solves #242
receive was easy but for fallback I rely on the originalText

@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #244 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #244   +/-   ##
=======================================
  Coverage   99.70%   99.70%           
=======================================
  Files          84       84           
  Lines         677      680    +3     
  Branches      115      116    +1     
=======================================
+ Hits          675      678    +3     
  Misses          2        2           

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 30d90a3...26d5009. Read the comment docs.

//
// An neat idea would be to rely on the pragma and enforce it but for the
// moment this will do.
return options.originalText.slice(
Copy link
Member

@mattiaerre mattiaerre Feb 29, 2020

Choose a reason for hiding this comment

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

should we enforce here that we are only returning function and fallback and not, for instance, facepalm (yes it's 8 letters)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do that but will mess with coverage though as there’s no chance for us to trigger the parser to have facepalm as a function definition

Copy link
Contributor Author

@Janther Janther Feb 29, 2020

Choose a reason for hiding this comment

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

ok I updated it to be a bit more explicit. I understand clever solutions might be too clever in the long run 😆

Copy link
Member

Choose a reason for hiding this comment

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

hahaha, I've pushed a commit, what do you think? something in between?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, this way we'll also know if solidity inserts a new keyword as undefined doesn't silently print in concat

Copy link
Member

Choose a reason for hiding this comment

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

I go ahead and merge then ok?

Copy link
Member

@fvictorio fvictorio left a comment

Choose a reason for hiding this comment

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

The parser should be the one giving more information here. I'll open an issue for that. But since this works fine, I would go ahead and merge it. If in the future the parser adds support for it, we can refactor this code.

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