Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/elixir_analyzer/constants.ex
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ defmodule ElixirAnalyzer.Constants do
captains_log_do_not_use_enum_random: "elixir.captains-log.do_not_use_enum_random",
captains_log_do_not_use_rand_uniform_real: "elixir.captains-log.do_not_use_rand_uniform_real",
captains_log_use_rand_uniform: "elixir.captains-log.use_rand_uniform",
captains_log_use_io_lib: "elixir.captains-log.use_io_lib",
captains_log_use_erlang: "elixir.captains-log.use_erlang",
Copy link
Contributor

Choose a reason for hiding this comment

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

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?


# Community Garden Comments
community_garden_use_get_and_update: "elixir.community-garden.use_get_and_update",
Expand Down
29 changes: 25 additions & 4 deletions lib/elixir_analyzer/test_suite/captains_log.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ defmodule ElixirAnalyzer.TestSuite.CaptainsLog do
"""
use ElixirAnalyzer.ExerciseTest
alias ElixirAnalyzer.Constants
alias ElixirAnalyzer.Source

assert_call "random_planet_class uses Enum.random" do
type :essential
Expand Down Expand Up @@ -43,10 +44,30 @@ defmodule ElixirAnalyzer.TestSuite.CaptainsLog do
suppress_if "random_stardate does not use Enum.random", :fail
end

assert_call "format_stardate uses :io_lib" do

check_source "format_stardate uses erlang" do
type :essential
calling_fn module: CaptainsLog, name: :format_stardate
called_fn module: :io_lib, name: :_
comment Constants.captains_log_use_io_lib()
comment Constants.captains_log_use_erlang()

check(%Source{code_ast: code_ast}) do
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

  1. 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 :)

{_, erlang?} =
Macro.prewalk(code_ast, false, fn node, acc ->
case node do
# usage :io_lib.format/2
{{:., _, [:io_lib, :format]}, _, _} ->
{node, true}

# usage :erlang.function_name/arity
# matches any function call from :erlang module
{{:., _, [:erlang, _]}, _, _} ->
{node, true}

_ ->
{node, acc}
end
end)

erlang?
end
end
end
2 changes: 1 addition & 1 deletion test/elixir_analyzer/test_suite/captains_log_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ defmodule ElixirAnalyzer.ExerciseTest.CaptainsLogTest do
end

test_exercise_analysis "format_stardate uses Float.round",
comments_include: [Constants.captains_log_use_io_lib()] do
comments_include: [Constants.captains_log_use_erlang()] do
Copy link
Contributor

Choose a reason for hiding this comment

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

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

defmodule CaptainsLog do
def format_stardate(stardate) do
if is_float(stardate) do
Expand Down
20 changes: 20 additions & 0 deletions test_data/captains-log/no_erlang_solution/.meta/config.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"authors": [
"angelikatyborska"
],
"contributors": [
"neenjaw"
],
"files": {
"solution": [
"lib/captains_log.ex"
],
"test": [
"test/captains_log_test.exs"
],
"exemplar": [
".meta/exemplar.ex"
]
},
"blurb": "Learn about randomness and using Erlang libraries from Elixir by helping Mary generate stardates and starship registry numbers for her Star Trek themed pen-and-paper role playing sessions."
}
20 changes: 20 additions & 0 deletions test_data/captains-log/no_erlang_solution/.meta/exemplar.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
defmodule CaptainsLog do
@planetary_classes ["D", "H", "J", "K", "L", "M", "N", "R", "T", "Y"]

def random_planet_class() do
Enum.random(@planetary_classes)
end

def random_ship_registry_number() do
number = Enum.random(1000..9999)
"NCC-#{number}"
end

def random_stardate() do
:rand.uniform() * 1000 + 41_000
end

def format_stardate(stardate) do
to_string(:io_lib.format("~.1f", [stardate]))
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"summary":"Check the comments for things to fix. 🛠","comments":[{"type":"essential","comment":"elixir.captains-log.use_erlang"},{"type":"informative","params":{"mentoring_request_url":"https://exercism.org/tracks/elixir/exercises/captains-log/mentor_discussions"},"comment":"elixir.general.feedback_request"}]}
20 changes: 20 additions & 0 deletions test_data/captains-log/no_erlang_solution/lib/captains_log.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
defmodule CaptainsLog do
@planetary_classes ["D", "H", "J", "K", "L", "M", "N", "R", "T", "Y"]

def random_planet_class() do
Enum.random(@planetary_classes)
end

def random_ship_registry_number() do
number = Enum.random(1000..9999)
"NCC-#{number}"
end

def random_stardate() do
:rand.uniform() * 1000 + 41_000
end

def format_stardate(stardate) do
Float.round(stardate, 1) |> to_string()
end
end
20 changes: 20 additions & 0 deletions test_data/captains-log/perfect_solution/.meta/config.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"authors": [
"angelikatyborska"
],
"contributors": [
"neenjaw"
],
"files": {
"solution": [
"lib/captains_log.ex"
],
"test": [
"test/captains_log_test.exs"
],
"exemplar": [
".meta/exemplar.ex"
]
},
"blurb": "Learn about randomness and using Erlang libraries from Elixir by helping Mary generate stardates and starship registry numbers for her Star Trek themed pen-and-paper role playing sessions."
}
20 changes: 20 additions & 0 deletions test_data/captains-log/perfect_solution/.meta/exemplar.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
defmodule CaptainsLog do
@planetary_classes ["D", "H", "J", "K", "L", "M", "N", "R", "T", "Y"]

def random_planet_class() do
Enum.random(@planetary_classes)
end

def random_ship_registry_number() do
number = Enum.random(1000..9999)
"NCC-#{number}"
end

def random_stardate() do
:rand.uniform() * 1000 + 41_000
end

def format_stardate(stardate) do
to_string(:io_lib.format("~.1f", [stardate]))
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"comments":[],"summary":"Submission analyzed. No automated suggestions found."}
20 changes: 20 additions & 0 deletions test_data/captains-log/perfect_solution/lib/captains_log.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
defmodule CaptainsLog do
@planetary_classes ["D", "H", "J", "K", "L", "M", "N", "R", "T", "Y"]

def random_planet_class() do
Enum.random(@planetary_classes)
end

def random_ship_registry_number() do
number = Enum.random(1000..9999)
"NCC-#{number}"
end

def random_stardate() do
:rand.uniform() * 1000 + 41_000
end

def format_stardate(stardate) do
:io_lib.format("~.1f", [stardate]) |> to_string()
end
end