Added erlang-rebar3 checker #1144

joedevivo · 2016-10-21T18:54:57Z

I wrote a flycheck checker for erlang projects using rebar3, but when I tried adding it to melpa, they said it was pretty basic, so I should try including it in flycheck, so here I am :D

As per the suggestion here: melpa/melpa#4342

CLAassistant · 2016-10-21T18:54:58Z

All committers have signed the CLA.

swsnr · 2016-10-23T15:38:49Z

@joedevivo Thank you very much. I'll leave a first review.

swsnr

Could you clarify how this checker should interact with the existing erlang checker? Should they be used together?

Also, could you perhaps try and write an integration test for this syntax checker? See test/flycheck-test.el for examples.

swsnr · 2016-10-23T15:42:01Z

flycheck.el

+
+(defun flycheck-rebar3-project-root (_checker)
+  "Return directory where =rebar.config= is located."
+  (locate-dominating-file buffer-file-name "rebar.config"))


Does rebar work if rebar.config is missing? If not it needs an :enabled function that checks whether the file is actually present.

swsnr · 2016-10-23T15:43:37Z

flycheck.el

+  "An Erlang syntax checker using the rebar3 build tool.
+
+See URL `https://www.rebar3.org'."
+  :command ("rebar3" "compile")


I presume the syntax checker compiles the entire project? The it needs a :predicate that checks whether the current buffer is saved because in this case Flycheck won't be able to feed the actual contents of the buffer to the checker, so if the buffer is not saved the errors will not be accurate.

joedevivo · 2016-10-23T15:45:52Z

@lunaryorn I just installed today's flycheck build and it's generating some errors:

Compiling file /Users/joe/.emacs.d/elpa/flycheck-20161023.643/flycheck-ert.el at Sun Oct 23 08:44:11 2016
Entering directory ‘/Users/joe/.emacs.d/elpa/flycheck-20161023.643/’

Compiling file /Users/joe/.emacs.d/elpa/flycheck-20161023.643/flycheck.el at Sun Oct 23 08:44:11 2016
flycheck.el:2610:1:Warning: Unused lexical argument ‘cwd’

In flycheck-finish-current-syntax-check:
flycheck.el:2630:20:Warning: reference to free variable ‘working-dir’

In flycheck-handle-idle-change:
flycheck.el:2748:8:Warning: function flycheck-handle-idle-change used to take
    0+ arguments, now takes 0

In end of data:
flycheck.el:9343:1:Warning: the function
    ‘flycheck-syntax-check-current-checker’ is not known to be defined.

Compiling no file at Sun Oct 23 08:44:11 2016

It's not related to this, but I figured you should know

swsnr · 2016-10-23T15:54:22Z

@joedevivo Thanks for making me aware, but that's already been fixed in f112d0c. Please don't hijack discussions for unrelated topics. Instead just open a new issue. Thank you.

- Still no tests, will take a stab at that next

swsnr · 2016-10-23T17:25:30Z

flycheck.el

+wouldn't be aware of. One such example is the erlang -include
+directive. Rebar3 is aware of the context for those includes,
+where the standard erlang checker is not. This checker should be
+used instead of the erlang checker for all rebar3 projects


Thanks but I didn't mean to clarify that in the docstring, but rather write a comment on the Github issue 😊 Please remove this rather lengthy paragraph again.

@joedevivo Can you please remove this paragraph?

swsnr · 2016-10-23T17:26:06Z

flycheck.el

  :working-directory flycheck-rebar3-project-root)

+(defun flycheck-rebar-config-exists-p ()
+  "Whether there is a rebar.config for the current buffer."
+  (let* ((executable (flycheck-find-checker-executable 'erlang-rebar3))


Is there a reason why you check that? Are you aware that Flycheck does that automatically?

swsnr · 2016-10-23T17:26:17Z

flycheck.el

+  "Whether there is a rebar.config for the current buffer."
+  (let* ((executable (flycheck-find-checker-executable 'erlang-rebar3))
+         (config-file (locate-dominating-file buffer-file-name "rebar.config"))
+         )


Please don't let parens hang on the next line.

joedevivo · 2016-10-23T17:53:55Z

I'm happy with this pull request except for one thing. My tests are failing, and it's because the rebar3 tests aren't saving the buffer. I can't figure out how to make it do this. Any suggestion?

swsnr · 2016-10-23T18:25:59Z

@joedevivo Can you show the output of the test?

And do we really need all those files? The separate LICENSE is a no-go, in any case.

joedevivo · 2016-10-23T18:34:04Z

@lunaryorn yes sorry, I'll take care of that and set it up so it's just the minimum rebar3 needs to recognize a project.

Here's my error output:

➜  flycheck git:(master) ✗ make LANGUAGE=erlang integ
cask exec emacs -Q --batch -L .  -l maint/flycheck-compile.el -f flycheck/batch-byte-compile flycheck-buttercup.el
cask exec emacs -Q --batch -L .  --load test/flycheck-test --load test/run.el -f flycheck-run-tests-main '(and (tag external-tool) (language erlang))'
Running tests on Emacs 25.1.1, built at 2016-09-17
Running 4 tests (2016-10-23 11:32:02-0700)
Loading erlang-skels...
Test flycheck-define-checker/erlang-rebar3/error backtrace:
  flycheck-ert-should-errors((7 nil error "head mismatch" :checker erl
  apply(flycheck-ert-should-errors (7 nil error "head mismatch" :check
  flycheck-ert-should-syntax-check("language/erlang-rebar3/src/erlang-
  (closure (python-indent-guess-indent-offset js3-mode-show-parse-erro
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test flycheck-define-checker/erlang-reba
  ert-run-or-rerun-test([cl-struct-ert--stats (and "flycheck-" (and (t
  ert-run-tests((and "flycheck-" (and (tag external-tool) (tag languag
  ert-run-tests-batch((and "flycheck-" (and (tag external-tool) (tag l
  ert-run-tests-batch-and-exit((and "flycheck-" (and (tag external-too
  (let ((selector (car-safe (prog1 argv (setq argv (cdr argv)))))) (if
  flycheck-run-tests-batch-and-exit()
  (let ((debug-on-error t)) (flycheck-run-tests-batch-and-exit))
  (let* ((load-prefer-newer t) (source-directory (locate-dominating-fi
  flycheck-run-tests-main()
  command-line-1(("-L" "." "--load" "test/flycheck-test" "--load" "tes
  command-line()
  normal-top-level()
Test flycheck-define-checker/erlang-rebar3/error condition:
    (ert-test-failed
     ((should
       (equal expected flycheck-current-errors))
      :form
      (equal
       ([cl-struct-flycheck-error #<killed buffer> erlang-rebar3 "/Users/joe/dev/joedevivo/flycheck/test/resources/language/erlang-rebar3/src/erlang-error.erl" 7 nil "head mismatch" error nil])
       nil)
      :value nil :explanation
      (different-types
       ([cl-struct-flycheck-error #<killed buffer> erlang-rebar3 "/Users/joe/dev/joedevivo/flycheck/test/resources/language/erlang-rebar3/src/erlang-error.erl" 7 nil "head mismatch" error nil])
       nil)))
   FAILED  1/4  flycheck-define-checker/erlang-rebar3/error
Test flycheck-define-checker/erlang-rebar3/warning backtrace:
  flycheck-ert-should-errors((6 nil warning "wrong number of arguments
  apply(flycheck-ert-should-errors (6 nil warning "wrong number of arg
  flycheck-ert-should-syntax-check("language/erlang-rebar3/src/erlang-
  (closure (python-indent-guess-indent-offset js3-mode-show-parse-erro
  ert--run-test-internal([cl-struct-ert--test-execution-info [cl-struc
  ert-run-test([cl-struct-ert-test flycheck-define-checker/erlang-reba
  ert-run-or-rerun-test([cl-struct-ert--stats (and "flycheck-" (and (t
  ert-run-tests((and "flycheck-" (and (tag external-tool) (tag languag
  ert-run-tests-batch((and "flycheck-" (and (tag external-tool) (tag l
  ert-run-tests-batch-and-exit((and "flycheck-" (and (tag external-too
  (let ((selector (car-safe (prog1 argv (setq argv (cdr argv)))))) (if
  flycheck-run-tests-batch-and-exit()
  (let ((debug-on-error t)) (flycheck-run-tests-batch-and-exit))
  (let* ((load-prefer-newer t) (source-directory (locate-dominating-fi
  flycheck-run-tests-main()
  command-line-1(("-L" "." "--load" "test/flycheck-test" "--load" "tes
  command-line()
  normal-top-level()
Test flycheck-define-checker/erlang-rebar3/warning condition:
    (ert-test-failed
     ((should
       (equal expected flycheck-current-errors))
      :form
      (equal
       ([cl-struct-flycheck-error #<killed buffer> erlang-rebar3 "/Users/joe/dev/joedevivo/flycheck/test/resources/language/erlang-rebar3/src/erlang-warning.erl" 6 nil "wrong number of arguments in format call" warning nil])
       nil)
      :value nil :explanation
      (different-types
       ([cl-struct-flycheck-error #<killed buffer> erlang-rebar3 "/Users/joe/dev/joedevivo/flycheck/test/resources/language/erlang-rebar3/src/erlang-warning.erl" 6 nil "wrong number of arguments in format call" warning nil])
       nil)))
   FAILED  2/4  flycheck-define-checker/erlang-rebar3/warning
   passed  3/4  flycheck-define-checker/erlang/error
   passed  4/4  flycheck-define-checker/erlang/warning

Ran 4 tests, 2 results as expected, 2 unexpected (2016-10-23 11:32:06-0700)

2 unexpected results:
   FAILED  flycheck-define-checker/erlang-rebar3/error
   FAILED  flycheck-define-checker/erlang-rebar3/warning

make: *** [integ] Error 1

joedevivo · 2016-10-23T18:36:44Z

I don't think I'll be able to act much more on this for the rest of the day, but any suggestions you have on the test front, I'll be happy to try out tonight or tomorrow, as well as finishing up the other recommendations here.

Thanks for taking the time today

swsnr · 2016-10-23T20:07:15Z

Me neither, I'll take a look at the test stuff tomorrow (my time 😉)

joedevivo · 2016-10-25T01:04:06Z

flycheck.el

+                          buffer-files)
+                ; If buffer-files is nil, it just means we didn't use
+                ; any temp files to compile this
+                (not buffer-files))


I added this or condition to make the tests pass. Since I wasn't building with any temporary files, buffer-files was always coming through nil, so seq-some would always be false. This resulted in no errors making it back to the assertion. It's weird that this worked fine in real world usage.

joedevivo · 2016-10-25T01:08:01Z

Confirming it still works in regular usage

See flycheck/flycheck#1144 for more info

swsnr · 2016-10-30T15:20:45Z

flycheck.el

+                                      0
+                                      (string-match
+                                       "\\(/_build/[^/]+/lib\\|/_checkouts\\)"
+                                       rebarconfig)))


@joedevivo Can you illustrate what this substring is supposed to do?

A rebar3 project will check dependencies out into a _build/<profile_name>/lib folder. Your dependency will have .erl files, and you may want to open them in emacs to review what they're doing. If you do, flycheck will try and build that rebar3 project, because it will satisfy the checker.

Let's look at a real world example. https://github.com/joedevivo/chatterbox

Chatterbox has src/*.erl and we'll wan't those to work as the checker was already defined.

When I build chatterbox, it pulls dependencies into _build/default/lib, but also _build/test/lib which lets me include specific dependencies that I don't want bundled with the release, they're just there to test. If I open up any file in _build/*/lib/*/src/*.erl, it shouldn't try and compile that project on its own. That would wind up building _build/<profile>/lib/<dependency>/_build/<profile>/lib/<dependency_of_the_first_depencency> and so on. This regex will match places that rebar checks things out to, and helps us make sure we're only building the parent project.

_checkouts is a little different. Chatterbox is coupled to another repo I maintain: https://github.com/joedevivo/hpack. The development is done in tandem, and I can work on a master version of hpack by checking it out to _checkouts.

I hope this clarifies what that regex is doing here.

@joedevivo I see, but I'm not convinced that a regex is the right tool to use here, and I do not like string substitutions over pathnames. That's just inherently fragile—for instance your current regex obviously breaks on windows pathnames.

How about the following algorithm instead:

Find the nearest project-directory containing rebar.config file with locate-dominating-file

From that file, find the nearest directory that contains a _build directory with locate-dominating-file. If none exists return project-directory as the final working directory.

If one exists let-bind it to default-directory and then recursively call flycheck-rebar3-project-root again to restart the search for the top-most rebar project directory.

What do you think?

I like it. The issue with step 2 is that I need to look for _build or _checkouts; however, there's an even better solution. We can just keep looking for rebar.config. Every rebar3 project needs one, so a recursive call to locate-dominating-file would be enough. I've rust run into one snag: A recursive call to locate-dominating-file finds the one in ., so I need the recursive call to run on the parent directory, and I just can't figure out the function to give me the parent-dir, do you know how to do that? Thanks for the elisp lesson :D

I'm going to push with a solution using (expand-file-name ".." possibly-nested-project-dir)

@joedevivo Mmh, I'm not sure whether that's not too eager: Is there no reason why anyone would ever nest two proper rebar projects under a top-level project? Would you never use "nested" projects with rebar?

Chances are, if flycheck encounters this then it's a mistake on the part of the user. Something got built in a way it wasn't supposed to, or some artifact of some build survived somewhere it shouldn't have. No matter the case, the top rebar.config it can find is always the right answer. I think something less eager would still work 90% of the time, so if we're concerned about speed we could opt for something like that.

There's no nesting of projects with rebar. If you are doing something like, there's a way to structure your source so it will build multiple applications, but if you did that, it would still be done with a single rebar.config file and the applications would be bundled together into a single release artifact.

The closest thing to what I think you're looking for would be if you had two rebar3 projects, and a third than included both of them as dependencies, but that's already covered.

Did that clear it up?

@joedevivo Yes, absolutely, thank you very much for taking the time to explain.

I'll take another look at the implementation, but maybe not before tomorrow.

Also thanks for bearing with me, for your patience with all my questions and remarks. It's a pleasure to review a pull request like this, thanks a lot!

You're welcome! I'm happy to give back, and it's been fun learning more about elisp.

swsnr · 2016-10-30T15:21:24Z

test/flycheck-test.el

+   "language/erlang/rebar3/_checkouts/dependency/src/dependency.erl" 'erlang-mode)
+  (should (not (file-exists-p
+                (flycheck-ert-resource-filename
+                 "language/erlang/rebar3/_build/default/lib/dependency/_build")))))


What's this assertion for? What does it test? Why is it important this this file/directory does not exist?

Based on the comment above about the regex, this assertion is testing that when we open a buffer for a dependency, it doesn't build the dependency as if it were a standalone project. If it did, it would create that _build directory, so we're asserting that it doesn't.

@joedevivo Can you add a comment that explains this above this assertion? E.g.

Ensure that the dependency file wasn't build as standalone project which would create a separate _build directory

would be sufficient (please wrap it to 80 characters though :) )

swsnr · 2016-10-30T15:22:23Z

flycheck.el

+  :command ("rebar3" "compile")
+  :error-parser
+  (lambda (output checker buffer)
+    (flycheck-parse-with-patterns (ansi-color-filter-apply output) checker buffer))


Please add a little comment here that explains why we need to filter colours manually, and include a link to this pull request, so that we can easily find the discussion again when we look at this code in future.

swsnr · 2016-10-30T15:23:54Z

flycheck.el

@@ -79,6 +79,7 @@
 (require 'help-mode)             ; `define-button-type'
 (require 'find-func)             ; `find-function-regexp-alist'
 (require 'json)                  ; `flycheck-parse-tslint'
+(require 'ansi-color)            ; strip ansi color from command output


Please move this require into the body of the rebar3 :error-parser function. That'll break the build due to a byte compiler warning, which we can then fix together.

As expected, there's a compilation error ``` In toplevel form: flycheck.el:9377:1:Error: the function ‘ansi-color-filter-apply’ might not be defined at runtime. make: *** [flycheck.elc] Error 1 ```

joedevivo · 2016-10-30T15:42:14Z

@lunaryorn I pushed up the changes, and yes, now there's a compile error.

swsnr · 2016-10-31T09:42:58Z

flycheck.el

+    ;; relevant disucssion can be found
+    ;; https://github.com/flycheck/flycheck/pull/1144
+    (require 'ansi-color)
+    (flycheck-parse-with-patterns (ansi-color-filter-apply output) checker buffer))


@joedevivo Thanks for the explanatory comment.

Now, to fix the compiler warning use the following as argument to flycheck-parse-with-patterns:

(and (fboundp 'ansi-color-filter-apply) (ansi-color-filter-apply output))

The fboundp guard is entirely redundant, because we know that (require 'ansi-color) brings that function in scope, but the byte compiler can't make that link. However, the fboundp guard convinces it that we're only calling the function when it's defined and thus removes the warning.

Ugly, but works, and much evil is achieved 😎

swsnr · 2016-10-31T09:44:21Z

test/flycheck-test.el

+   "language/erlang/rebar3/_checkouts/dependency/src/dependency.erl" 'erlang-mode)
+  (should (not (file-exists-p
+                (flycheck-ert-resource-filename
+                 "language/erlang/rebar3/_build/default/lib/dependency/_build")))))


@joedevivo Can you add a comment that explains this above this assertion? E.g.

Ensure that the dependency file wasn't build as standalone project which would create a separate _build directory

would be sufficient (please wrap it to 80 characters though :) )

swsnr · 2016-10-31T09:49:51Z

flycheck.el

@@ -6920,6 +6920,41 @@ See URL `http://www.erlang.org/'."
   (error line-start (file-name) ":" line ": " (message) line-end))
  :modes erlang-mode)

+(defun flycheck-rebar3-project-root (&optional _checker)
+  "Return directory where =rebar.config= is located."
+  (let* ((rebarconfig (locate-dominating-file buffer-file-name "rebar.config"))


By the way, the name of this variable is wrong, I think. Afaik locate-dominating-file returns the directory containing the file, not the path to the file itself, so the variable doesn't actually contain the path to the rebar configuration file but rather the project directory.

So I guess project-directory is the better name here isn't it?

swsnr · 2016-10-31T09:55:35Z

flycheck.el

+                                      0
+                                      (string-match
+                                       "\\(/_build/[^/]+/lib\\|/_checkouts\\)"
+                                       rebarconfig)))


@joedevivo I see, but I'm not convinced that a regex is the right tool to use here, and I do not like string substitutions over pathnames. That's just inherently fragile—for instance your current regex obviously breaks on windows pathnames.

How about the following algorithm instead:

Find the nearest project-directory containing rebar.config file with locate-dominating-file

From that file, find the nearest directory that contains a _build directory with locate-dominating-file. If none exists return project-directory as the final working directory.

If one exists let-bind it to default-directory and then recursively call flycheck-rebar3-project-root again to restart the search for the top-most rebar project directory.

What do you think?

swsnr

Sorry for the long delay, had a unexpectedly busy week.

I overlooked a markup issue in the docstrings which needs to be fixed, and I'm not yet happy with the code that finds the project root. I think it can be simplified, maybe with a single while loop that traverses upwards until there's no project root directory anymore.

swsnr · 2016-11-07T10:50:34Z

flycheck.el

@@ -6920,6 +6920,49 @@ See URL `http://www.erlang.org/'."
   (error line-start (file-name) ":" line ": " (message) line-end))
  :modes erlang-mode)

+(defun locate-rebar3-project-root (file-name)
+  "Return directory where the root =rebar.config= for the source FILE-NAME is located."


I just noticed that you're using org-mode syntax in docstrings, i.e. =…=. However docstrings are not in Org syntax; please go over all docstrings and remove all occurrences of this syntax.

Also, I think you can shorten the summary of this function and instead write a more comprehensive docstring, e.g.

Find the top-most rebar project root for source FILE-NAME. A project root directory is any directory containing a rebar.config file. Find the top-most directory to move out of any nested dependencies. FILE-NAME is a source file for which to find the project. Return the absolute path to the directory.

The first line of docstrings should be very brief as it's used in many listing with limited space, e.g. apropos, so it's better to write a short summary line and a more verbose docstring.

swsnr · 2016-11-07T11:13:18Z

flycheck.el

+              (locate-dominating-file
+               (expand-file-name ".." possibly-nested-project-dir)
+               "rebar.config")
+            nil)


I've been thinking about whether we could simplify this code, to only call locate-dominating-file once. Would that be possible?

Hopefully the new implementation looks good.

swsnr · 2016-11-14T10:15:57Z

flycheck.el

@@ -80,7 +80,6 @@
 (require 'find-func)             ; `find-function-regexp-alist'
 (require 'json)                  ; `flycheck-parse-tslint'

-


Why this change?

swsnr · 2016-11-14T10:16:20Z

flycheck.el

+      (locate-rebar3-project-root (directory-file-name current-dir)
+                                  file-name
+                                  (cons (contains-rebar-config current-dir) acc))
+      )))


Please don't let parenthesis hang on a line on their own. Always join with the code before.

swsnr · 2016-11-14T10:18:20Z

@joedevivo Ok, there are two minor stylistic issues left, but other than that I think we're ready now.

Would you like to join Flycheck as part of a new Erlang team to take care of Erlang support in Flycheck? That'd really help us to maintain proper support for Erlang.

joedevivo · 2016-11-17T11:09:28Z

Done. Would you like me to squash the commits?

I'm happy to keep working on Erlang support as the need arises :D

swsnr · 2016-11-17T15:02:01Z

@joedevivo Yup, please squash your commits.

Thanks for joining us, I'll invite you to the team tonight, and approve the review. Then you'll be able to merge this PR yourself 👍

Meanwhile browse through our Contributor's Guide and our Maintainer's Guide 😊

joedevivo · 2016-11-20T16:43:17Z

@lunaryorn I squashed the commits a few days ago, but forgot to let you know. sorry about that.

joedevivo · 2016-11-27T12:36:07Z

@lunaryorn hey, I haven't gotten the team invite, do you mind sending again?

Simplify · 2017-05-08T19:50:18Z

@cpitclaudel Can you take look at this PR? I'll like to have it in Flycheck.

@joedevivo Sorry about delay. Are you still willing to support Erlang in Flycheck?

cpitclaudel · 2017-05-11T19:33:55Z

flycheck.el

@@ -6920,6 +6921,62 @@ See URL `http://www.erlang.org/'."
   (error line-start (file-name) ":" line ": " (message) line-end))
  :modes erlang-mode)

+(defun contains-rebar-config (dir-name)
+  "Return DIR-NAME if DIR-NAME/rebar.config exists, nil otherwise."
+  (if (file-exists-p (expand-file-name "rebar.config" dir-name))


This should be when or and instead of if

cpitclaudel · 2017-05-11T19:34:09Z

flycheck.el

+  (if (file-exists-p (expand-file-name "rebar.config" dir-name))
+      dir-name nil))
+
+(defun locate-rebar3-project-root (file-name &optional prev-file-name acc)


This seems to be essentially locate-dominating-file, but looking for the topmost file. Why is that needed?

The first rebar.config it finds isn't necessarily the project root. This function is actually slightly wrong though. It should use "\\(/_build/[^/]+/lib\\|/_checkouts\\|/apps\\)"

To be clearer, any Erlang application in a project could have a rebar.config of its own, while there will also be a top level rebar.config. So this just makes sure that the file we are using isn't a sub-app or dependency of the project.

Yes. You might want to explore how a dependency works, but that buffer might think it's the main project without this. Then it would try and fetch the dependencies dependencies, which have already been fetched by the root project.

@tsloughter as implemented, this should catch all of those things. I assume that the critique here is that it might catch more than it should?

@cpitclaudel, @tsloughter is one of the authors of rebar3, so I'll defer to his judgement on if this needs further modification. Or, as I hope, that it's good enough for now, we can get this merged, and then come back and make small tweaks as needed.

@tsloughter Thanks, that's what I meant by looking for the topmost file.

cpitclaudel · 2017-05-11T19:34:36Z

flycheck.el

+
+Return the absolute path to the directory"
+  (if (string= file-name prev-file-name)
+      (car (seq-drop-while (lambda (x) (not x)) acc))


No need for seq-drop-while here; it should be enough to use (remove nil …)

cpitclaudel · 2017-05-11T19:35:02Z

flycheck.el

+    ;; rebar3 outputs ANSI terminal colors, which don't match up with
+    ;; :error-patterns, so we strip those color codes from the output
+    ;; here before passing it along to the default behavior. The
+    ;; relevant disucssion can be found


found → found at

cpitclaudel · 2017-05-11T19:35:46Z

flycheck.el

+    ;; https://github.com/flycheck/flycheck/pull/1144
+    (require 'ansi-color)
+    (flycheck-parse-with-patterns
+     (and (fboundp 'ansi-color-filter-apply) (ansi-color-filter-apply output))


This looks suspicious. Wouldn't ansi-color-filter-apply always be bound, due to the require above?

I don't really understand this either. @lunaryorn told me to add this here

This is the only comment I left outstanding, maybe you'll have a better context for this after reading the comment I mentioned and we can go from there

cpitclaudel · 2017-05-11T19:36:44Z

flycheck.el

+   (error line-start
+          (file-name) ":" line ": " (message) line-end))
+  :modes erlang-mode
+  :enabled (lambda () (flycheck-rebar3-project-root))


No need to η-expand the flycheck-rebar3-project-root (you can drop the lambda).

cpitclaudel · 2017-05-11T19:36:52Z

flycheck.el

+          (file-name) ":" line ": " (message) line-end))
+  :modes erlang-mode
+  :enabled (lambda () (flycheck-rebar3-project-root))
+  :predicate (lambda () (flycheck-buffer-saved-p))


cpitclaudel · 2017-05-11T19:38:18Z

test/flycheck-test.el

+(flycheck-ert-def-checker-test erlang-rebar3 erlang build
+  (flycheck-ert-should-syntax-check
+   "language/erlang/rebar3/_checkouts/dependency/src/dependency.erl" 'erlang-mode)
+  ;; Ensure that the dependency file wasn't build as standalone


build → built

cpitclaudel · 2017-05-11T19:38:35Z

test/resources/language/erlang/rebar3/.gitignore

+logs
+_build
+.idea
+rebar.lock


Do we really need all of these?

cpitclaudel · 2017-05-11T19:38:41Z

test/resources/language/erlang/rebar3/_checkouts/dependency/.gitignore

+logs
+_build
+.idea
+rebar.lock


Both were generated by rebar3. I reduced them to things these integration tests actually create.

cpitclaudel · 2017-05-11T19:39:28Z

I left a few comments — there are some typos, and I'm a bit confused about some parts of the code.

tsloughter · 2017-05-21T03:53:56Z

@joedevivo want me to take this over?

joedevivo · 2017-05-21T03:56:59Z

I was going to take a look tomorrow morning. It seems like there's not much left. :D

tsloughter · 2017-05-21T03:59:04Z

Ok cool :), had just seen there wasn't much since 2016.

Still fuzzy on what to do about [this](flycheck#1144 (comment))

joedevivo · 2017-05-21T15:32:02Z

@cpitclaudel I'm happy to maintain this still

cpitclaudel · 2017-05-22T03:03:54Z

flycheck.el

+    ;; rebar3 outputs ANSI terminal colors, which don't match up with
+    ;; :error-patterns, so we strip those color codes from the output
+    ;; here before passing it along to the default behavior. The
+    ;; relevant disucssion can be found at


disucssion → discussion

cpitclaudel · 2017-05-22T03:05:26Z

flycheck.el

+    ;; https://github.com/flycheck/flycheck/pull/1144
+    (require 'ansi-color)
+    (flycheck-parse-with-patterns
+     (and (fboundp 'ansi-color-filter-apply) (ansi-color-filter-apply output))


I think the idea of this check was to silence the byte-compiler — I'd rather use with-no-warnings, or add a comment about the condition.

cpitclaudel · 2017-05-22T03:06:39Z

flycheck.el

+  (if (file-exists-p (expand-file-name "rebar.config" dir-name))
+      dir-name nil))
+
+(defun locate-rebar3-project-root (file-name &optional prev-file-name acc)


@tsloughter Thanks, that's what I meant by looking for the topmost file.

cpitclaudel · 2017-05-22T03:19:05Z

flycheck.el

+      (locate-rebar3-project-root
+       (directory-file-name current-dir)
+       file-name
+       (cons (contains-rebar-config current-dir) acc)))))


I think it would help with maintainability if this function was based on locate-dominating-file. Something like this, maybe? (untested):

(defun locate-topmost-dominating-file (file name) (let ((result nil) (further-up file)) (while further-up (setq further-up (locate-dominating-file further-up name)) (when further-up (setq result further-up) (setq further-up (file-name-directory (directory-file-name further-up))))) result))

cpitclaudel · 2017-05-22T03:22:05Z

Thanks a lot. Things look great now, and this PR's discussion thread is starting to look suspiciously long :) Welcome aboard, and thanks for this contribution! Sorry for the long reviewing process.

I left a few comments — it would be great if you could address them in a separate PR. Additionally, it would be nice to ask the rebar3 folks to update the checker to add a flag to disable color coding (or better, they could restrict color codes to TTYs).

joedevivo force-pushed the master branch 2 times, most recently from e338f23 to a194ed6 Compare October 21, 2016 19:21

swsnr added component: checkers status: ready for review labels Oct 21, 2016

joedevivo force-pushed the master branch from a194ed6 to ecc33cc Compare October 22, 2016 11:24

joedevivo force-pushed the master branch from ecc33cc to bc72041 Compare October 23, 2016 15:42

swsnr suggested changes Oct 23, 2016

View reviewed changes

swsnr added status: pending and removed status: ready for review labels Oct 23, 2016

joedevivo added a commit to joedevivo/flycheck that referenced this pull request Oct 23, 2016

Enhancements based on flycheck#1144

0010148

- Still no tests, will take a stab at that next

swsnr reviewed Oct 23, 2016

View reviewed changes

joedevivo force-pushed the master branch from 7f34e3b to 82ccdb6 Compare October 23, 2016 17:42

joedevivo force-pushed the master branch from 82ccdb6 to 73ac20c Compare October 23, 2016 18:35

swsnr added status: ready for review and removed status: pending labels Oct 24, 2016

joedevivo commented Oct 25, 2016

View reviewed changes

joedevivo force-pushed the master branch 2 times, most recently from fdb2f2f to c4190ab Compare October 25, 2016 01:16

joedevivo added a commit to joedevivo/flycheck-rebar3 that referenced this pull request Oct 30, 2016

Incorporate changes from joedevivo/flycheck

56a7c94

See flycheck/flycheck#1144 for more info

swsnr removed the status: pending label Oct 30, 2016

swsnr suggested changes Oct 30, 2016

View reviewed changes

swsnr suggested changes Oct 31, 2016

View reviewed changes

swsnr suggested changes Nov 7, 2016

View reviewed changes

swsnr suggested changes Nov 14, 2016

View reviewed changes

Added erlang-rebar3 checker

0085697

joedevivo force-pushed the master branch from 2e4a00e to 0085697 Compare November 17, 2016 15:27

cpitclaudel reviewed May 11, 2017

View reviewed changes

Incorporated most of @cpitclaudel's feedback on flycheck#1144

3e59d5e

Still fuzzy on what to do about [this](flycheck#1144 (comment))

cpitclaudel reviewed May 22, 2017

View reviewed changes

cpitclaudel approved these changes May 22, 2017

View reviewed changes

cpitclaudel merged commit 9bf7337 into flycheck:master May 22, 2017

velimir mentioned this pull request May 30, 2017

erlang-rebar3 doesn't check rebar version #1258

Closed

dgud mentioned this pull request Apr 4, 2019

flycheck-rebar3 can be removed melpa/melpa#6110

Closed

berbiche mentioned this pull request Nov 13, 2020

flycheck-rebar3 (package not found) has been merged into flycheck doomemacs/doomemacs#4264

Closed

		@@ -80,7 +80,6 @@
		(require 'find-func) ; `find-function-regexp-alist'
		(require 'json) ; `flycheck-parse-tslint'

Added erlang-rebar3 checker #1144

Added erlang-rebar3 checker #1144

Conversation

joedevivo commented Oct 21, 2016

CLAassistant commented Oct 21, 2016 • edited Loading

swsnr commented Oct 23, 2016

swsnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joedevivo commented Oct 23, 2016

swsnr commented Oct 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joedevivo commented Oct 23, 2016

swsnr commented Oct 23, 2016

joedevivo commented Oct 23, 2016

joedevivo commented Oct 23, 2016

swsnr commented Oct 23, 2016

Choose a reason for hiding this comment

joedevivo commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joedevivo commented Oct 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swsnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swsnr commented Nov 14, 2016

joedevivo commented Nov 17, 2016

swsnr commented Nov 17, 2016

joedevivo commented Nov 20, 2016

joedevivo commented Nov 27, 2016

Simplify commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpitclaudel commented May 11, 2017

tsloughter commented May 21, 2017

joedevivo commented May 21, 2017

tsloughter commented May 21, 2017

CLAassistant commented Oct 21, 2016 •

edited

Loading