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?

[FraSanga]

exercism/website-copy#2395

I cannot ask anyone for a review on the repo.

[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?

[Copilot]

Pull request overview

This PR updates the Captains Log analyzer rule for format_stardate/1 to allow Erlang-based implementations (per issue #340), switching from a call-based assertion to an AST-based source check.

Changes:

• Replaced the format_stardate analyzer check with a check_source that scans the submission AST for Erlang usage.
• Renamed the Captains Log analyzer comment id from use_io_lib to use_erlang.
• Updated the Captains Log test to expect the new comment id.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
lib/elixir_analyzer/test_suite/captains_log.ex	Replaces the format_stardate check with an AST traversal looking for :io_lib/:erlang calls.
lib/elixir_analyzer/constants.ex	Renames the analyzer comment id used for the Captains Log format_stardate guidance.
test/elixir_analyzer/test_suite/captains_log_test.exs	Updates the expectation to the renamed comment id.

Comments suppressed due to low confidence (1)

test/elixir_analyzer/test_suite/captains_log_test.exs:110

• There’s no test covering the newly-allowed format_stardate implementations that use :erlang (e.g., :erlang.float_to_binary/2) and should not emit this comment. Add a test_exercise_analysis case that uses an Erlang-based implementation and asserts Constants.captains_log_use_erlang() is excluded, so the analyzer change is protected against regressions.

  test_exercise_analysis "format_stardate uses Float.round",
    comments_include: [Constants.captains_log_use_erlang()] do
    defmodule CaptainsLog do
      def format_stardate(stardate) do
        if is_float(stardate) do
          Float.round(stardate, 1) |> to_string()
        else
          raise ArgumentError
        end
      end
    end

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[FraSanga]

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.

[jiegillet]

Thanks for the quick follow up, I didn't take 5 months to reply this time :)

We are on the right track, but your concern is warranted, so let's address it.

[jiegillet]

You are right that we should not be sending two (contradictory) comments to someone that didn't use an Erlang function.
Let's make it so that both assert_call for format_stardate send the same comment Constants.captains_log_use_erlang(), but that comment should be generic and say that we expect the use of an Erlang function, any function, not the one from the :erlang module specifically.

And then we can dump the more specific Constants.captains_log_use_io_lib() comment.

[jiegillet]

Great job, thank you very much for your contribution.

I jumped in and added a quick fix, because I merged the sister PR a little too early and I didn't want to leave a broken state for too long, sorry about that.