Allow reading multiple file solutions and exemploids #298
Conversation
jiegillet
added
x:module/analyzer
| @@ -0,0 +1,5 @@ | |||
| defmodule SquareRoot.Cheating do | |||
| def calculate(n) do | |||
| Float.pow(n / 1, 0.5) |> floor() | |||
There was a problem hiding this comment.
This worked straight away, which I was quite happy about.
| fn path, {:ok, code} -> | ||
| case File.read(path) do | ||
| {:ok, file} when is_nil(code) -> {:cont, {:ok, file}} | ||
| {:ok, file} -> {:cont, {:ok, code <> "\n" <> file}} |
There was a problem hiding this comment.
This line brings the coverage down a bit, I can't test it because we don't have a multiple file exemploid.
lib/elixir_analyzer.ex
Outdated
| %{"example" => path} -> {:practice, path} | ||
| end | ||
|
|
||
| exemploid_path = |
There was a problem hiding this comment.
The singular variable names confuse me now... may I suggest renaming?
solution_path->solution_fileseditor_path->editor_filesexemploid_path- >exemploid_filescode_path->all_compiled_filesorall_analyzable_filesorall_files_for_analysis? "code" is very vague 🤔 (I know, you didn't choose this name initially)
Maybe the keys in the Source struct should be renamed too.
There was a problem hiding this comment.
Yes, I got lazy there :D
I left code_string and code_ast in Source as they are, still vague but not really confusing in context.
lib/elixir_analyzer.ex
Outdated
| params.path | ||
| |> Path.join("lib") | ||
| |> ls_r() | ||
| |> Enum.filter(&String.ends_with?(&1, ".ex")) |
There was a problem hiding this comment.
I initially read ls_r as "list files right" and didn't get it for a while... maybe the function could be named ls_recursive?
On the other hand, I wonder if this function can be replaced by:
Path.join([params.path, "lib", "**", "*.ex"]) |> Path.wildcard()
| {:ok, nil}, | ||
| fn path, {:ok, code} -> | ||
| case File.read(path) do | ||
| {:ok, file} when is_nil(code) -> {:cont, {:ok, file}} | ||
| {:ok, file} -> {:cont, {:ok, code <> "\n" <> file}} |
There was a problem hiding this comment.
I assume that the case is_nil(code) is only there because of the first iteration and initial value of the accumulator. What if we did something like this, appending new lines instead of prepending? We would end up with one extra newline at the very end, but it shouldn't matter, right? (suggested code not tested)
| {:ok, nil}, | |
| fn path, {:ok, code} -> | |
| case File.read(path) do | |
| {:ok, file} when is_nil(code) -> {:cont, {:ok, file}} | |
| {:ok, file} -> {:cont, {:ok, code <> "\n" <> file}} | |
| {:ok, ""}, | |
| fn path, {:ok, code} -> | |
| case File.read(path) do | |
| {:ok, file} -> {:cont, {:ok, code <> file <> "\n"}} |
There was a problem hiding this comment.
It almost doesn't matter, but in some unlikely cases it makes a line in an error message off-by-one (see CI fail before reverted commit). Line number kind of loses meaning when you have multiple files, but after reflection, I'd still like to keep the nil.
lib/elixir_analyzer.ex
Outdated
| params.path | ||
| |> Path.join("lib") | ||
| |> ls_r() | ||
| |> Enum.filter(&String.ends_with?(&1, ".ex")) |
lib/elixir_analyzer.ex
Outdated
| %{"example" => path} -> {:practice, path} | ||
| end | ||
|
|
||
| exemploid_path = |
There was a problem hiding this comment.
Yes, I got lazy there :D
I left code_string and code_ast in Source as they are, still vague but not really confusing in context.
This reverts commit b0b04ce.
2 participants
Closes #276.
The analyzer can now accept multiple file exemploids (as long as they are listed in the config file) and multiple file solutions (even if they are not mentioned in the config file).
This gives more freedom to exercise builders, but also allows students to structure their solution as they please (especially CLI users) while still benefiting from the analyzer advice.
The compiler warning module has been changed to handle multiple file compilation.