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: vyper lint #479

Closed
wants to merge 9 commits into from
Closed

feat: vyper lint #479

wants to merge 9 commits into from

Conversation

0xbeedao
Copy link

This is a forked version of flake8-vyper, updated to work for version 2.16 of Vyper, and with a Github Action to automate the linting. It is submitted for the open ticket #16 , adding linting for Vyper.

@0xbeedao 0xbeedao changed the title Feat/vyper lint feat: vyper lint Oct 24, 2021
contracts/Registry.vy Outdated Show resolved Hide resolved
@pandadefi
Copy link
Contributor

pandadefi commented Oct 25, 2021

Would it be possible for you turn flake8-vyper-2 into a requirement?

@0xbeedao
Copy link
Author

Would it be possible for you turn flake8-vyper-2 into a requirement?

I could. I wasn't sure whether it is worth it, because of the unfortunate tying between the minor version and the linter.

I would do something like flake8-vyper-2-16.

@0xbeedao
Copy link
Author

I will go ahead and package this up as a requirement. In the meantime, I think it is worth pointing out that due to the nature of this exact PR, it will never pass linting. That's because it is correctly finding a whole bunch of errors. See the link for the lint run above. The output has the lint errors.

Great to see it proved out, actually.

@0xbeedao
Copy link
Author

0xbeedao commented Nov 1, 2021

I have made the linting now get installed via a simple Pip installation, rather than adding that code to this repo, and also made the Github Action for the linter able to handle the two different Vyper versions in use.

Note that this will never pass Linting, due to the nature of this task. I can't take responsibility for editing your .vy files to pass the Linter, at least not on this first round.

@pandadefi
Copy link
Contributor

pandadefi commented Nov 1, 2021

Can you fix the files that are falling linting on this PR?
Can you make it so that if registry linting fails it will still do the vault linting and not stop immediately ?

@0xbeedao
Copy link
Author

0xbeedao commented Nov 1, 2021

Can you fix the files that are falling linting on this PR? Can you make it so that if registry linting fails it will still do the vault linting and not stop immediately ?

I think so, I'll give it a shot.


API_VERSION: constant(String[28]) = "0.4.3"

from vyper.interfaces import ERC20

implements: ERC20

Copy link
Author

Choose a reason for hiding this comment

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

I had to move this to the top to get the constants recognized by the linter

@@ -242,8 +271,8 @@ SECS_PER_YEAR: constant(uint256) = 31_556_952 # 365.2425 days
# `nonces` track `permit` approvals with signature.
nonces: public(HashMap[address, uint256])
DOMAIN_SEPARATOR: public(bytes32)
DOMAIN_TYPE_HASH: constant(bytes32) = keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)')
PERMIT_TYPE_HASH: constant(bytes32) = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)")
Copy link
Author

Choose a reason for hiding this comment

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

I couldn't chop these to 80 characters.

@0xbeedao
Copy link
Author

That was really a lot of lint, mostly minor stuff.

@@ -6,3 +6,6 @@ reports/
node_modules/
.test_durations
yarn-error.log
flake8.log
*egg-info/
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this from?

Copy link
Author

Choose a reason for hiding this comment

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

The log is auto-generated by flake8 when it runs
Installing Flake8-vyper-linter via the commandline ended up creating a "flake8...egg-info" file, so I added it to ignore.
That line would be safe to remove, since the egg files are transient.

contracts/Registry.vy Outdated Show resolved Hide resolved
@0xbeedao
Copy link
Author

Pushed an update which should fix those # dev changes.

@pandadefi
Copy link
Contributor

@0xbeedao the vyper lint tooling will be a bottleneck for future dev since we are upgrading the vyper version when they get released #494

We would need to have flake8 properly maintained and vyper version agnostic in order to add linting to our CI.

@pandadefi pandadefi closed this Dec 2, 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.

2 participants