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

bug: solhint ordering #274

Closed
gitpusha opened this issue Nov 27, 2020 · 7 comments · Fixed by #275
Closed

bug: solhint ordering #274

gitpusha opened this issue Nov 27, 2020 · 7 comments · Fixed by #275
Labels

Comments

@gitpusha
Copy link

"solhint": "3.3.2",

"ordering": "error",

doesnt seem to work e.g here.

@gitpusha
Copy link
Author

In general solhint 's ordering rule seems to diverge a fair bit from the solidity style guide ones.

I believe solhint ordering tries to enforce:

  1. external
  2. external view/pure
  3. public
  4. public view/pure
  5. internal
  6. private

when I think the rule set should be (according to solidity docs):

  1. external
  2. external view
  3. external pure
  4. public
  5. public view
  6. public pure
  7. internal
  8. internal view
  9. internal pure
  10. private
  11. private view
  12. private pure

right?

I guess too much of a pain to implement the granularity but I guess you could simplify and still be much more compliant with the official style guide like so:

  1. external
  2. external view/pure
  3. public
  4. public view/pure
  5. internal
  6. internal view/pure
  7. private
  8. private view/pure

@gitpusha gitpusha changed the title bug: solhint ordering doesnt always seem to work for view bug: solhint ordering Dec 14, 2020
@fernandomg
Copy link
Contributor

Hey @gitpusha, you're right we're missing the view/pure modifiers for internal and private functions.

I'll give it a look and come back to you with an approach.

Thank you!

@fernandomg fernandomg added the bug label Dec 16, 2020
@gitpusha
Copy link
Author

gitpusha commented Dec 17, 2020

Hi @fernandomg thanks for your time!

doesnt seem to work e.g here.
Also the initial issue what seems to be an actual bug in the existing rule set because solhint doesnt complain about public view coming before external payable here for example.

You can clone the repo and run yarn lint:sol to see that it doesnt complain. I noticed this deficiency in the ordering rule in many other places too.

Also, you guys should open a gitcoin grant :)

@fernandomg
Copy link
Contributor

fernandomg commented Dec 17, 2020

@gitpusha, So, in your particular case, the ordering rule didn't recognize the using for directive.

So when it reached that contract part, it should have thrown an error message like "Unrecognized contract part, please report this issue". That is not happening, I still need to fix it.

On the other hand, I managed to adapt the rule to support the most granular approach:

  1. external
  2. external view
  3. external pure
  4. public
  5. public view
  6. public pure
  7. internal
  8. internal view
  9. internal pure
  10. private
  11. private view
  12. private pure

I'll let you know when I have something deliverable.

Thank you

@fernandomg
Copy link
Contributor

@gitpusha, please feel free to review and validate #275

@gitpusha
Copy link
Author

gitpusha commented Jan 6, 2021

@fernandomg I did a review and it looks correct to me :) When do you think you will merge?

mergify bot pushed a commit to gnosis/gp-v2-contracts that referenced this issue Mar 15, 2021
Bumps [solhint](https://github.com/protofire/solhint) from 3.3.3 to 3.3.4.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/protofire/solhint/releases">solhint's releases</a>.</em></p>
<blockquote>
<h2>v3.3.4</h2>
<p>This release fixes an issue with the <code>ordering</code> rule <a href="https://github.com/protofire/solhint/issues/274">#274</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/protofire/solhint/commit/991b8755ef2babcc2c97c278b039c86abeef4b8a"><code>991b875</code></a> 3.3.4</li>
<li><a href="https://github.com/protofire/solhint/commit/21732ff2a3fafd413e96998b2598fbca249537aa"><code>21732ff</code></a> Merge pull request <a href="https://github.com/protofire/solhint/issues/275">#275</a> from protofire/fix/274-ordering</li>
<li><a href="https://github.com/protofire/solhint/commit/6ca955ed72edafc0b3c30a2590b483c32553acf4"><code>6ca955e</code></a> add an ordering-correct example for the <code>receive</code> function usage</li>
<li><a href="https://github.com/protofire/solhint/commit/b9dcfd621891810548e5a99191b3bdccea3bb9f2"><code>b9dcfd6</code></a> update ordering docs</li>
<li><a href="https://github.com/protofire/solhint/commit/afb215b88d5ef9ed3c3afa4b52d4e1aad4fee96e"><code>afb215b</code></a> add tests for <code>ordering</code> rule</li>
<li><a href="https://github.com/protofire/solhint/commit/5b107987b540a53378df03efbb9aa5bcb9480309"><code>5b10798</code></a> fix ordering rule</li>
<li>See full diff in <a href="https://github.com/protofire/solhint/compare/v3.3.3...v3.3.4">compare view</a></li>
</ul>
</details>
<details>
<summary>Maintainer changes</summary>
<p>This version was pushed to npm by <a href="https://www.npmjs.com/~fernandomg">fernandomg</a>, a new releaser for solhint since your current version.</p>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=solhint&package-manager=npm_and_yarn&previous-version=3.3.3&new-version=3.3.4)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually


</details>
@gitpusha
Copy link
Author

thx for the merge @fernandomg 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants