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

function-order now also applies to public view/pure, not just external. #94

Merged
merged 2 commits into from Apr 24, 2019
Merged

function-order now also applies to public view/pure, not just external. #94

merged 2 commits into from Apr 24, 2019

Conversation

nventuro
Copy link
Contributor

Added for consistency's sake. If accepted, we may also want to include internal and private here. Distinction between view and pure is also missing, is that something we want to tackle?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 99.528% when pulling 7ef5fb9 on nventuro:function-order-public-constant into f389547 on protofire:master.

@fvictorio
Copy link
Contributor

Thanks @nventuro, and sorry for taking so long to check this.

Distinction between view and pure is also missing, is that something we want to tackle?

I'm not against this, but it might be a constraint too harsh. Maybe it could be an extra option for the rule?

In any case, what should be the order? I guess pure functions should be after view functions, since they are more "const".

@nventuro
Copy link
Contributor Author

nventuro commented Jan 2, 2019

constant was removed in Solidity 0.5.0, so I don't think that'd be 'too harsh'.

The ordering you suggested makes sense, and is in line with the Solidity style guide.

@fvictorio
Copy link
Contributor

constant was removed in Solidity 0.5.0, so I don't think that'd be 'too harsh'.

I meant forcing certain order for view and pure functions; not allowing to alternate view and pure functions, for example.

@nventuro
Copy link
Contributor Author

nventuro commented Jan 2, 2019

Ah, in that case I'd consider making it optional, yes. I don't feel too strongly about function order though, so I may not be the best person to ask about this 😛

@fvictorio fvictorio merged commit 95f54d3 into protofire:master Apr 24, 2019
@fvictorio
Copy link
Contributor

@nventuro Sorry for not merging this before. I'll make a release now so that it's available.

@nventuro nventuro deleted the function-order-public-constant branch April 24, 2019 21:22
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.

None yet

3 participants