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

Print returns in function types at the end #333

Merged
merged 1 commit into from
Sep 5, 2020
Merged

Conversation

fvictorio
Copy link
Member

Fixes #325
Fixes #330

}
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@adjicf @andrekorol can you check if this looks good to you?

Copy link

@andrekorol andrekorol Sep 3, 2020

Choose a reason for hiding this comment

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

I've cloned the function-types branch and successfully ran the Jest tests locally.

But when running prettier with the function-types branch installed on node_modules/prettier-plugin-solidity, I still get the same ParserError:
contracts/Pyramid.sol[error] contracts/Pyramid.sol: ParserError: extraneous input 'pure' expecting {',', ')'} (10:44)

I wonder if that's a problem with the Solidity parser (federicobond/solidity-parser-antlr) used by this plugin.

I noticed that the tests on AllSolidityFeatures.sol are using pragma solidity ^0.4.0;, but the contract I'm getting the ParserError on was using pragma solidity >=0.4.16 <0.8.0;, and the version of solc I was using to compile it is 0.7.0. I'm not sure, but another possibility is that this error only arises when using solc-0.7.x.

This is nothing critical or urgent though, the contract in question is from the Solidity documentation and can be found in this section: Function Types.

Copy link
Member

Choose a reason for hiding this comment

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

I'll merge as is and maybe we can have a follow-up PR?

Choose a reason for hiding this comment

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

That's fine by me :)

I would have to take a closer look at the problem before creating a new PR, but from quick experimentation, the problem only seems to come up when using solc-0.7.

Copy link
Member Author

Choose a reason for hiding this comment

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

@andrekorol can you open an issue if that's still happening to you? And btw, if you think it's a bug with the parser (which it's likely), you can open an issue in https://github.com/solidity-parser/parser too

Choose a reason for hiding this comment

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

@fvictorio Sure! No problem :)

@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #333 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #333   +/-   ##
=======================================
  Coverage   99.71%   99.71%           
=======================================
  Files          88       88           
  Lines         693      693           
  Branches      130      130           
=======================================
  Hits          691      691           
  Misses          2        2           
Impacted Files Coverage Δ
src/nodes/FunctionTypeName.js 100.00% <ø> (ø)

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 76687a2...e6fc3e0. Read the comment docs.

Copy link
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.

👍

}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

I'll merge as is and maybe we can have a follow-up PR?

@mattiaerre mattiaerre merged commit 8c64418 into master Sep 5, 2020
@mattiaerre mattiaerre deleted the function-types branch September 5, 2020 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants