Added erlang ast test in captains-log

[FraSanga]

As described in issue #340, I modified the file by attempting to perform the check with the AST.

I am uncertain if this is correct, but the tests I added also work with the examples I created.

As soon as I get the go-ahead, I'll also make a PR on website-copy.
I hope this has been helpful. 😄

[jiegillet]

Thank you for the PR!
Before I go into a detailed review, could I ask you to address two things:

1. I don't understand why all the files you are modifying are completely redefined. As far as I could tell, you are only modifying a few lines, not the whole files. Could you figure it out and fix it? It would really help with the review and the git history.
2. The tests in test_data are used as smoke tests for the CI, we don't typically add new ones in there unless we have a good reason. The tests you added in test/elixir_analyzer/test_suite/captains_log_test.exs should be enough.

[FraSanga]

It should be fixed now.
Every time Windows messes up the formatting at the end of the line.

[FraSanga]

It has been open for 5 months. Can someone take a look?

[angelikatyborska]

Hi there! We did completely forget about your PR, sorry about that 😞 I'm writing this comment to confirm that both @jiegillet and I saw your recent reminder and we'll looking for some free time to re-review the PR soon.

[jiegillet]

Sorry about the wait. I think I was waiting for you to address my second point, and then lost track of the PR altogether.

2. The tests in test_data are used as smoke tests for the CI, we don't typically add new ones in there unless we have a good reason. The tests you added in test/elixir_analyzer/test_suite/captains_log_test.exs should be enough.

This still stands, could you revert the changes to test_data?

Other than that, I've looked at your changes and left some comments.

[jiegillet]

This value is a path to a file in this repo that contains the text of the analyzer comment, so you need to create a new file over there for the analyzer to work. Could you do the appropriate change over there, link to the PR in this PR's description and request a review from me?

[jiegillet]

I'm hesitating about this change.
It doesn't check that "format_stardate uses erlang", it checks that one of two specific Erlang functions are used anywhere at all, even in dead code.
I can think of two preferable approaches:

1. The easy one: add an extra assert_call (the following code is missing values, it's just to show the structure)

assert_call "format_stardate uses :io_lib" do
  calling_fn module: CaptainsLog, name: :format_stardate
  called_fn module: :io_lib, name: :_
  suppress_if "format_stardate uses :erlang", :pass
end

assert_call "format_stardate uses :erlang" do
  calling_fn module: CaptainsLog, name: :format_stardate
  called_fn module: :erlang, name: :_
  suppress_if "format_stardate uses :io_lib", :pass
end

this should be equivalent to the intent of your PR.

2. The hard one: add a feature to the macro assert_call, something like

assert_call "format_stardate uses an Erlang module" do
  calling_fn module: CaptainsLog, name: :format_stardate
  called_fn module: AnyErlangModule, name: :_
end

This would detect the use of any non-elixir module, while preserving all of the advantages of assert_call (tracking imports, aliases, the use of helper functions...). It would also be future proof in case there is another Erlang module that could solve the problem. I think it should be possible since Elixir module names are always capitalized unlike erlang ones, but this is the hard approach because the assert_call macro is quite complex, so I'm not pushing for it, just writing down my thoughts.

Since approach 1. would already be a strict improvement, I think that's the one we should aim at to relieve your from your 5-month journey within a reasonable time :)

[jiegillet]

Could you add at one test that use an :erlang function to solve the problem and doesn't raise a comment?