elixir-analyzer icon indicating copy to clipboard operation
elixir-analyzer copied to clipboard

Added erlang ast test in `captains-log`

Open FraSanga opened this issue 5 months ago • 2 comments

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. 😄

FraSanga avatar Sep 16 '25 22:09 FraSanga

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.

jiegillet avatar Sep 27 '25 10:09 jiegillet

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

FraSanga avatar Sep 27 '25 11:09 FraSanga

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

FraSanga avatar Feb 12 '26 01:02 FraSanga

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.

angelikatyborska avatar Feb 16 '26 07:02 angelikatyborska

I should have sorted everything out. My only concern with the first approach is that if a user makes a mistake, both the comments from :io_lib and :erlang will be displayed, I suppose.

FraSanga avatar Feb 24 '26 14:02 FraSanga