Skip to content

Fix prettier removing payable from address payable#77

Merged
mattiaerre merged 4 commits intoprettier-solidity:masterfrom
svechinsky:feature/address-payable
Jan 6, 2019
Merged

Fix prettier removing payable from address payable#77
mattiaerre merged 4 commits intoprettier-solidity:masterfrom
svechinsky:feature/address-payable

Conversation

@svechinsky
Copy link
Copy Markdown
Contributor

So I noticed an issue with the new address payable type.
At first the plugin would fail because the published version of solidity-parser-antlr had a bug and didn't recognize the payable keyword.
After using the most recent version from git I noticed that the printer would just remove the payable keyword.

So this PR fixes this.
I have absolutley no idea if what I did here is the correct way to fix it but it seems to work for me.
Also I have no idea what's the best way to include the most recent version of solidity-parser-antlr, maybe best to ask the maintainer to publish the newest version?

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 21, 2018

Codecov Report

Merging #77 into master will decrease coverage by 0.97%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   85.43%   84.46%   -0.98%     
==========================================
  Files           5        5              
  Lines         206      206              
  Branches       45       45              
==========================================
- Hits          176      174       -2     
- Misses         28       30       +2     
  Partials        2        2
Impacted Files Coverage Δ
src/printer.js 83.03% <100%> (ø) ⬆️
src/clean.js 50% <0%> (-50%) ⬇️

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 002a4da...62fde9e. Read the comment docs.

@svechinsky
Copy link
Copy Markdown
Contributor Author

Fixed some styling issues, also federicobond/solidity-parser-antlr#40 the new version was released so this should be mergable

@fvictorio
Copy link
Copy Markdown
Member

@svechinsky could you add tests for this?

@mattiaerre
Copy link
Copy Markdown
Member

mattiaerre commented Dec 27, 2018

thanks for this PR @svechinsky if you can add a test that would be really appreciated if you cannot please comment w/ an example from a contract where you've noticed the issue and we'll add the test for you as we'd like to make sure that the fix doesn't introduce any regression as well as improve the overall code coverage

// cc @fvictorio

@svechinsky
Copy link
Copy Markdown
Contributor Author

svechinsky commented Dec 27, 2018 via email

@mattiaerre
Copy link
Copy Markdown
Member

thanks @svechinsky

@svechinsky
Copy link
Copy Markdown
Contributor Author

So if you run prettier on this function the payable part will be removed (both in the parameters as well as in the declaration).

contract AddressPayable{
 function sendSomeEth(address payable to){
   address payable target = to;
   target.transfer(100 eth);
}
}

@mattiaerre
Copy link
Copy Markdown
Member

hi @svechinsky can you grant me write access to your fork so that I can push the test? otherwise you can add the attached folder to the project under tests

AddressPayable.zip

@mattiaerre mattiaerre added the bug Something isn't working label Dec 28, 2018
@mattiaerre mattiaerre added this to the beta launch milestone Dec 28, 2018
@fvictorio
Copy link
Copy Markdown
Member

Added the tests. @mattiaerre, just FYI, you can push to a forked repo when there's a pull request like this 🙂

@fvictorio
Copy link
Copy Markdown
Member

This closes #78, right?

Copy link
Copy Markdown
Member

@mattiaerre mattiaerre left a comment

Choose a reason for hiding this comment

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

this LGTM thanks @fvictorio

@mattiaerre mattiaerre merged commit 440057b into prettier-solidity:master Jan 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants