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

Add elixir support via dogma #969

Merged
merged 1 commit into from
Aug 31, 2016
Merged

Conversation

kthelgason
Copy link
Member

@kthelgason kthelgason commented May 17, 2016

This PR adds support for the Elixir language (as discussed in #877).
This relies on dogma for syntax checking, and so does not execute the code being checked as elixirc would.

Edit (@lunaryorn): Connects to #877

TODO:

  • Add predicate to make sure checker is not loaded when mix dogma is not available.
  • Parse dogma's JSON output format.
  • Tests for JSON parsing

(message) line-end))
:modes elixir-mode)


Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this blank line :)

@kthelgason
Copy link
Member Author

@lunaryorn removed blank line and force-pushed :) thanks for taking a look.

"An Elixir syntax checker using the Dogma analysis tool.

See URL `https://github.com/lpil/dogma/'."
:command ("mix" "dogma" "--format=flycheck" 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.

--format=flycheck?! 😊

dogma seems to support JSON output, though, and we generally prefer to parse structured output formats, which tend to be less fragile than plain text.

Would you mind to update this pull request to use an :error-parser that parses the JSON output of dogma?

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way how does mix know that dogma is available? Can we somehow test that to make sure that we don't enable this syntax checker when mix is there but dogma is not?

Copy link
Contributor

Choose a reason for hiding this comment

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

And last but not least, do we need source-inplace, or can we feed the buffer contents to dogma via standard input?

@swsnr
Copy link
Contributor

swsnr commented May 17, 2016

@kthelgason Thank you very much! It's great news to have Elixir back in, thanks for your work!

I've left a couple of comments on the diff, would you take a look?

@kthelgason
Copy link
Member Author

Thanks for the review @lunaryorn! As for your comments:

Would you mind to update this pull request to use an :error-parser that parses the JSON output of dogma?

I can do this, but it will involve more code and be more complicated. Here is an example of the JSON output:

{
  "metadata": {
    "dogma_version": "0.3.0",
    "elixir_version": "1.0.5",
    "erlang_version": "Erlang/OTP 10 [erts-7.0.3] [64-bit]",
    "system_architecture": "x86_64-apple-darwin14.5.0"
  },
  "files": [{
      "path": "lib/foo.ex",
      "errors": []
   }, {
      "path": "lib/bar.ex",
      "errors": [{
          "line": 1,
          "rule": "ModuleDoc",
          "message": "Module without @moduledoc detected"
       }, {
          "line": 14,
          "rule": "ComparisonToBoolean",
          "message": "Comparison to a boolean is useless"
       }
      ]
  }],
  "summary": {
    "error_count": 2,
    "inspected_file_count": 2
  }
}

It's up to you wether you think it's worth it :)

By the way how does mix know that dogma is available? Can we somehow test that to make sure that we don't enable this syntax checker when mix is there but dogma is not?

This is an excellent point, and I don't really have a good answer, perhaps you have some ideas as to how to accomplish this?

And last but not least, do we need source-inplace, or can we feed the buffer contents to dogma via standard input?

Unfortunately dogma does not currently support reading the source from stdin, but there is an open PR that implements that functionality. I can keep an eye on it and submit another PR when it lands.

@swsnr
Copy link
Contributor

swsnr commented May 17, 2016

@kthelgason

It's up to you wether you think it's worth it :)

Yes, I do. Take a look at the tslint checker to see an example for JSON parsing. And—if you've a little more time to spare—maybe also take a look at test/specs/languages/test-typescript.el to see a test case for JSON parsing, and probably add a test case for parsing dogma errors? 😊

This is an excellent point, and I don't really have a good answer, perhaps you have some ideas as to how to accomplish this?

Flycheck checkers can supply a :predicate to add additional checks. Take a look at the go-vet syntax checker when looks at the installed Go tools to find out whether vet is available. Can we do something similar for mix? Is there a mix commands or similar which lists all available mix commands?

Unfortunately dogma does not currently support reading the source from stdin, but there is an open PR that implements that functionality. I can keep an eye on it and submit another PR when it lands.

Would you mind to link this PR here so that we can track its process, too?

Meanwhile, would source instead of source-inplace work? We generally prefer the former because it keeps temporary files out of the source tree…

@kthelgason
Copy link
Member Author

Certainly, I'll find some time to work on this in the coming days and ping you when I have something. Thanks for the suggestions!

Flycheck checkers can supply a :predicate to add additional checks. Take a look at the go-vet syntax checker when looks at the installed Go tools to find out whether vet is available. Can we do something similar for mix? Is there a mix commands or similar which lists all available mix commands?

Yup, this would totally work.

Would you mind to link this PR here so that we can track its process, too?

lpil/dogma#194

Meanwhile, would source instead of source-inplace work? We generally prefer the former because it keeps temporary files out of the source tree…

Yes, that should definitely work as well. I'll change that.

@swsnr
Copy link
Contributor

swsnr commented May 17, 2016

@kthelgason Great, thank you very much 👍

@kthelgason
Copy link
Member Author

Update:
I'm pretty much done rewriting the checker to use dogma's json format. I ran into some issues with the checker itself and I'm working to get them upstreamed. I'm still waiting to resolve some issues before I can fully test the code I've written here. Will update again when I've gotten this resolved.

@kthelgason
Copy link
Member Author

Update:

There were some issues with using dogma as a mix command so I've added support for running dogma standalone. I'm going to submit a PR to dogma to get those changes in. Meanwhile I've updated this PR to work with that version of dogma. I've been running it in this configuration for a while and it seems to work well.

I apologise in advance if I'm doing something stupid in this PR, this is the first real code I write in elisp.

cc @lunaryorn

@swsnr
Copy link
Contributor

swsnr commented Jul 15, 2016

@kthelgason May I ask what's the status of this? Is the change to dogma already submitted? Is this PR ready for being merged?

@kthelgason
Copy link
Member Author

@lunaryorn Absolutely. The change has been pulled into dogma, so the code in this PR works with the current version. As for whether it's ready to be merged, you tell me 😄 from my perspective it's done.

@swsnr
Copy link
Contributor

swsnr commented Aug 18, 2016

Looks like you committed with a wrong mail address 😊

@kthelgason
Copy link
Member Author

I screwed up some settings and had to to battle with the CLA bot, luckily I won 😄 I cherry-picked @sergv's commit and tested it, I can confirm that the checker works brilliantly.

Also refactored according to the review, thanks for that @lunaryorn, great comments as usual!

@swsnr
Copy link
Contributor

swsnr commented Aug 19, 2016

@kthelgason Awesome, great news. It's ready to be merged then.

But please remove @sergv's commit from this PR again, as we'll merge it separately n #1040 before merging this one. Finally please squash all commits into a single one.

Sorry for being such a nuisance to you 😌

@kthelgason
Copy link
Member Author

@lunaryorn Done and done! Thanks for helping me out with this @lunaryorn and @sergv

"Come up with a suitable default directory to run CHECKER in.

This will either be the directory that contains `mix.exs' or,
if no such file is found in the directory hierarchy, ths directory
Copy link
Member

Choose a reason for hiding this comment

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

ths → the?

@cpitclaudel
Copy link
Member

This looks very good. I commented on the diff about a few typos.
Question: why are the functions called flycheck-elixir rather than flycheck-dogma? (this could be a convention that I'm just not aware of :)

@kthelgason
Copy link
Member Author

@cpitclaudel I'll try to address your comments later today or tomorrow, thanks for looking over the PR.
The reason I went with that naming (inspired by some other checker, can't recall which now) was that I hope to add support for more elixir checkers (credo) soon, in which case functions such as flycheck-elixir--find-default-directory could be shared.

@swsnr
Copy link
Contributor

swsnr commented Aug 25, 2016

I don't mind the name; these functions looks sufficiently generic imho to be named after the language.

@cpitclaudel
Copy link
Member

Thanks for the clarification (and for your efforts!). Everything LGTM modulo the small typos :)

@kthelgason
Copy link
Member Author

I fixed the typos and tried to come up with excuses for the comments I ignored 😄
Please give this one last look, Thanks!

@cpitclaudel
Copy link
Member

Looks perfect. @lunaryorn?

@swsnr
Copy link
Contributor

swsnr commented Aug 30, 2016

Uh, sh**, I knew I had forgotten a pull request for the last release 😢

LGTM, but unfortunately CHANGES.rst is now out of date as we're now heading towards Flycheck 30. 😞

@kthelgason I'm so sorry to bother you again but could you just update CHANGES.rst to note the change under Flycheck 30 before you merge? I'm sorry for all the nuisance 😞

@kthelgason
Copy link
Member Author

Don't worry about it! I fixed CHANGES.rst (I think) and pushed. 😄

@swsnr
Copy link
Contributor

swsnr commented Aug 31, 2016

Feel free to merge then 😊

@kthelgason kthelgason merged commit e10f40c into flycheck:master Aug 31, 2016
@kthelgason kthelgason deleted the elixir-support branch August 31, 2016 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants