Conditionally add flag to solium invocation & tweak output parsing for newer solium

[rowanthorpe]

I have labeled this "WIP" for now because it is presently only a "works for me" patch for working with my version of solium (1.1.8), and I am sure there needs to be some version-handling added to make this still handle output from older versions too, rather than only the newer one. When I find time I will try to add version-handling backward-compatibility, but that might take a while, so please add a commit of your own to do that if you don't want to wait.

Aside from updating the parsing for the new output format, I also needed to add the reporter flag because the default output reporter is now "pretty".

[LefterisJP]

Hello @rowanthorpe, thank you for the PR.

I don't mind waiting for proper handling of previous versions. I don't have the time to test it and add it on my own (mostly because I don't use solium lately so can't test it).

Whenever you can do it we can merge the PR so that I don't break backwards compatibility.

[rowanthorpe]

I found that trying to version-match against solium was too brittle, so chose to match against the presence (or not) of the --reporter flag in the --help output of solium. While getting there I did various changes, the most invasive of which was switching the logic from:

flycheck-define-checker (solidity)
flycheck-define-checker (solium)
when (solium checker active) {
  if (solium executable exists) {
    add-to-list (solium)
  }
}
when (solidity checker active) {
  if (solidity executable exists) {
    add-to-list (solidity)
  }
}

to:

when (solium checker active) {
  if (solium executable exists) {
    flycheck-define-checker (solium)
    add-to-list (solium)
  }
}
when (solidity checker active) {
  if (solidity executable exists) {
    flycheck-define-checker (solidity)
    add-to-list (solidity)
  }
}

The main reason for the restructure was the need to reuse the result of the solium --help test rather than calling out to the shell multiple times. In order to use that conditional, I also needed to pre-expand the macro for solium-checker (like you did for solidity-checker) from:

(flycheck-define-checker solium-checker ...)

to:

(flycheck-def-executable-var solium ...) +
 (flycheck-define-command-checker 'solium-checker ...)

for the same reason (needing to eval part of some args rather than passing verbatim text to the underlying functions). Happily, the restructure makes it more efficient as well, avoiding unnecessary definitions if the tool is set inactive or the executable doesn't exist.

Although I didn't find any "coding convention" docs, I am aware you may not agree with some of the edits I did, even if just for style reasons. If so, let me know what you want changed.

[rowanthorpe]

I revised the last commit to use clearer naming of a var (new-solium -> solium-has-reporter).

[LefterisJP]

Hey Rowan. Thank you very much for your work.

First off some questions.

Does this commit, moving checker-definitions inside the (when *-active improve efficiency in a measurable way? I don't dislike the change. Just curious.

Then you have another commit which moves them inside the call checking for existence of the relevant tool. Why have both commits? I guess the second alone is enough?

I guess when you refer to efficiency you mean this?

The main reason for the restructure was the need to reuse the result of the solium --help test rather than calling out to the shell multiple times.

Now for the styling, I looked at the code and it seems fine. Seems you even removed some rogue tabs. I have no idea how they ended up in there. My style in elisp is rather simple and I am not anal about it, so as long as the emacs elisp linter is not complaining I am fine.

I tested your patch in my local machine and it seems to work fine and the logic for supporting older versions of solium also looks good.

[rowanthorpe]

Does this commit, moving checker-definitions inside the (when *-active improve efficiency in a measurable way? I don't dislike the change. Just curious.

In this case, by "efficiency" I mean "solidity-mode-hook load-time efficiency" as opposed to "runtime efficiency once editing is under way", i.e. for each checker - if it is not set to be active then no load-time will be spent defining that checker (which wouldn't be used anyway). There is also the runtime memory-efficiency of having fewer symbols defined, but that is obviously way less important than the saved load-time aspect.

Then you have another commit which moves them inside the call checking for existence of the relevant tool. Why have both commits? I guess the second alone is enough?

I kept those two changes separate purely in an attempt to make the commits as readable and quickly auditable as possible for you. Seeing you have now grokked them, if you prefer I squash the two commits together just let me know and I will do it (on the other hand our future selves and other future contributors might prefer easier readability versus fewer git-refs, but whether separate or squashed commits are more readable is a style-choice which I will leave to you).

I guess when you refer to efficiency you mean this?

The main reason for the restructure was the need to reuse the result of the solium --help test rather than calling out to the shell multiple times.

Aside from the small reduction of load-time and tiny reduction of runtime-memory already mentioned above, the primary and much bigger "efficiency" reason for the restructuring is - as you correctly observe - being able to cache the result of the commandline call for reuse.

Now for the styling, I looked at the code and it seems fine. Seems you even removed some rogue tabs. I have no idea how they ended up in there. My style in elisp is rather simple and I am not anal about it, so as long as the emacs elisp linter is not complaining I am fine.

The majority of the rogue tabs were in the (emacs-lisp-macroexpand) output (which would also explain the not-for-human-consumption formatting in that same output). You probably copy-pasted it in verbatim for easy comparison while retaining the original unexpanded macro in commented form, so it is not a surprise the output looked like that. The other few "rogue" tabs would just be from edits made in Emacs without indent-tabs-mode set to nil (which some people prefer, hence why I asked about your style preferences).

I tested your patch in my local machine and it seems to work fine and the logic for supporting older versions of solium also looks good.

I decided to make this change based on flag-presence in solium rather than solium version number (i.e. "use --reporter gcc and different output parsing for solium versions with the --reporter flag"). This is because it started to look like parsing of the solium output would have incrementally failed in various solium commits leading up to that point (rather than at one pivotal moment). I didn't have the tenacity to track and compensate for every historical possibly-breaking-change in solium output (like here and here), but at least this way if someone is using a version of solium "new" enough to have unparsable output, but old enough to not have the --reporter flag yet, the solution can just be "upgrade your version of solium to one after November-2016 / v0.2.0". Perhaps you want to add that advice to the README to prevent repeated queries from users of ancient solium, or maybe the expectation is that very few would still be using such old versions...

[LefterisJP]

I didn't have the tenacity to track and compensate for every historical possibly-breaking-change in solium output (like here and here)

Oh no worries about that. I really don't think that the package needs to cater for really old versions of solium. Probably the easiest fix is to always tell them to upgrade solium or ... if they really want support for a specific version to open a PR here.

Perhaps you want to add that advice to the README to prevent repeated queries from users of ancient solium, or maybe the expectation is that very few would still be using such old versions...

Yeah I should do that.

if you prefer I squash the two commits

I normally favour squashed commits, but here it's fine as is. Thank you.

---

I will merge this patch now. Thank you for your work and for your detailed commenting and answers to my questions.