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

Simplify checkers and add solhint #75

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Conversation

landakram
Copy link
Contributor

Hi again. I made this branch a while ago when I started on a project that uses solhint. I added solhint support and also reworked the flycheck checker initialization.

The main reason I wanted to rework flycheck checker initialization is that flycheck has its own logic to enable or disable checkers based on the presence of the :command binary, and also handles locally installed linters with its flycheck-some-checker-executable variables. So we can simplify this package's configuration by always defining the checker, and letting flycheck handle enabling/disabling it.

The second reason is basically #41. I'd expect to be able to require the package and set variables it defines after it is loaded. The changes in this PR mean I don't need to set the solidity-flycheck-some-checker-active variables at all. But for other configuration, I can set them after loading the package itself (I'm using use-package here, but it should work with regular require too):

(use-package solidity-flycheck
    :straight (solidity-flycheck :type git :host github :repo "ethereum/emacs-solidity")
    :hook solidity-mode
    :config
    (setq solidity-flycheck-chaining-error-level t)
    (setq solidity-flycheck-use-project t))

Note: this would be a breaking change. If you're onboard with the direction, I can invest a little more time in updating the readme / thoroughly testing, though I've been running it fine with the above configuration for a few months. Otherwise, I could split the solhint support into a separate PR. I just happened to combine the changes when I implemented this many months ago.


Commit messages copied here:

  • Add a checker for solhint
  • Simplify checkers. This commit removes some of the load-time logic that
    determined whether to define the flycheck checkers. Flycheck has its own logic to
    enable or disable checkers based on the presence of the :command binary, making
    these checks redundant. Some of the other checks done at load time have been
    moved into (eval ...) forms, so that the binaries can be customized after load-time
    with flycheck's pre-defined vars e.g. `flycheck-solium-checker-executable'. This is
    useful when linting binaries are installed locally in a project.

- Add a checker for solhint
- Simplify checkers. This commit removes some of the load-time logic that
  determined whether to define the flycheck checkers. Flycheck has its own logic to
  enable or disable checkers based on the presence of the :command binary, making
  these checks redundant. Some of the other checks done at load time have been
  moved into (eval ...) forms, so that the binaries can be customized after load-time
  with flycheck's pre-defined vars e.g. `flycheck-solium-checker-executable'. This is
  useful when linting binaries are installed locally in a project.
Copy link
Collaborator

@LefterisJP LefterisJP left a comment

Choose a reason for hiding this comment

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

Oh wow thanks a lot for the big PR.

I unfortunately don't have time to actually test it as I don't use solidity a lot lately.

I would not mind the approach personally so if you could test it more and add docs that would be great. Also perhaps if you can check some other user who would use it and see it does not break everything that would make things simpler for merging.

@@ -37,5 +37,11 @@
:type 'string
:package-version '(solidity . "0.1.4"))

(defcustom solidity-solhint-path "solhint"
"Path to the solium binary."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Path to the solium binary."
"Path to the solhint binary."

(flycheck-def-executable-var solhint-checker "solhint")
(flycheck-define-command-checker 'solhint-checker
"A Solidity linter using solhint"
:command `(,solidity-solhint-path "-f" "visualstudio" source-inplace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:command `(,solidity-solhint-path "-f" "visualstudio" source-inplace)
:command `(,solidity-solhint-path "-f" "unix" source-inplace)

I don't see visualstudio among the formatter options. Although when it works when I run solhint -f visualstudio ./**/*.sol, would it make more sense here to use the unix formatted output?

Then the error-patterns would be something like:

  :error-patterns `((error
                     line-start (file-name) ":"  line ":" column ":" (zero-or-more " ")
                     "error" (zero-or-more " ") (message))
                    (warning
                     line-start (file-name) ":"  line ":" column ":" (zero-or-more " ")
                     "warning" (zero-or-more " ") (message)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to tweak your suggestion slightly to differentiate between errors and warnings. But should be good to go.

@clarkhenry
Copy link
Contributor

@landakram - could you check on my suggestions? Would love to get this merged. This PR works for me.

The unix formatter is documented, while the visualstudio formatter is not. Prefer the
unix formatter.
@landakram
Copy link
Contributor Author

@clarkhenry Apologies for the delay. I've made your suggested fixes and I believe the PR is ready to merge now.

@hrkrshnn
Copy link
Member

@clarkhenry, @GokhanPolat can you review this PR and approve it? I can merge it after that.

Copy link

@GokhanPolat GokhanPolat left a comment

Choose a reason for hiding this comment

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

These changes exactly same as my PR (it was already @landakram's code) which are I walked same way with same steps. @landakram did a good job.

@hrkrshnn
Copy link
Member

Thanks @GokhanPolat for the review. I'll proceed to merge this.

@hrkrshnn hrkrshnn merged commit 929ce58 into ethereum:master Oct 24, 2022
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.

5 participants