Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run common checks for every single solution; Add common check for snake_case #102

Merged
merged 18 commits into from
Jul 10, 2021
Merged
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
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ As a demonstration, once iex loads with `iex -S mix` you can type `ElixirAnalyze
### `ElixirAnalyzer`

- The is the main application module. A function call to `start_analyze/3` begins the analysis (either through IEX or the CLI escript [if generated]).
- A configuation in `config/config.exs` holds data for each exercise supported
- A configuration in `config/config.exs` holds data for each exercise supported

```elixir
config :elixir_analyzer,
Expand Down Expand Up @@ -110,7 +110,7 @@ Contains macro to generate a function returning the comment path on the `exercis

### `ElixirAnalyzer.CLI`

This module is a module for the CLI escript to parse the command line arguements and start the processing
This module is a module for the CLI escript to parse the command line arguments and start the processing

### `ElixirAnalyzer.ExerciseTest.________`

Expand Down
18 changes: 8 additions & 10 deletions lib/elixir_analyzer.ex
Original file line number Diff line number Diff line change
Expand Up @@ -134,15 +134,6 @@ defmodule ElixirAnalyzer do
analysis_module: analysis_module
}
rescue
e in ArgumentError ->
Logger.warning("TestSuite halted, ArgumentError", error_message: e.message)

submission
|> Submission.halt()
|> Submission.set_halt_reason(
"Analysis skipped, no analysis suite exists for this exercise"
)

e in File.Error ->
Logger.warning("Unable to decode 'config.json'", error_message: e.message)

Expand All @@ -156,6 +147,13 @@ defmodule ElixirAnalyzer do
submission
|> Submission.halt()
|> Submission.set_halt_reason("Analysis skipped, not able to decode solution config.")

e ->
Logger.warning("TestSuite halted, #{e.__struct__}", error_message: e.message)

submission
|> Submission.halt()
|> Submission.set_halt_reason("Analysis skipped, unexpected error #{e.__struct__}")
end
end

Expand All @@ -168,7 +166,7 @@ defmodule ElixirAnalyzer do
code_path = Path.dirname(full_code_path)
code_file = Path.basename(full_code_path)

{code_path, code_file, exercise_config[:analyzer_module]}
{code_path, code_file, exercise_config[:analyzer_module] || ElixirAnalyzer.TestSuite.Default}
end

# Else, use passed in params to analyze
Expand Down
3 changes: 2 additions & 1 deletion lib/elixir_analyzer/comment.ex
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ defmodule ElixirAnalyzer.Comment do
comment: String.t(),
type: :essential | :actionable | :informative | :celebratory,
status: :skip | :test,
suppress_if: false | {String.t(), :pass | :fail}
suppress_if: false | {String.t(), :pass | :fail},
params: map()
}
end
2 changes: 2 additions & 0 deletions lib/elixir_analyzer/constants.ex
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ defmodule ElixirAnalyzer.Constants do
solution_use_moduledoc: "elixir.solution.use_module_doc",
solution_use_specification: "elixir.solution.use_specification",
solution_raise_fn_clause_error: "elixir.solution.raise_fn_clause_error",
solution_module_attribute_name_snake_case: "elixir.solution.module_attribute_name_snake_case",
solution_function_name_snake_case: "elixir.solution.function_name_snake_case",

# Concept exercises

Expand Down
3 changes: 3 additions & 0 deletions lib/elixir_analyzer/exercise_test.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ defmodule ElixirAnalyzer.ExerciseTest do

alias ElixirAnalyzer.ExerciseTest.Feature.Compiler, as: FeatureCompiler
alias ElixirAnalyzer.ExerciseTest.AssertCall.Compiler, as: AssertCallCompiler
alias ElixirAnalyzer.ExerciseTest.CommonChecks

alias ElixirAnalyzer.Submission
alias ElixirAnalyzer.Constants
Expand Down Expand Up @@ -45,10 +46,12 @@ defmodule ElixirAnalyzer.ExerciseTest do
{:ok, code_ast} ->
feature_results = unquote(feature_tests) |> filter_suppressed_results()
assert_call_results = unquote(assert_call_tests) |> filter_suppressed_results()
common_checks_results = CommonChecks.run(code_ast, code_as_string)

submission
|> append_test_comments(feature_results)
|> append_test_comments(assert_call_results)
|> append_test_comments(common_checks_results)

{:error, e} ->
append_analysis_failure(submission, e)
Expand Down
18 changes: 18 additions & 0 deletions lib/elixir_analyzer/exercise_test/common_checks.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
defmodule ElixirAnalyzer.ExerciseTest.CommonChecks do
@moduledoc """
This module aggregates all common checks that should be run on every single solution.
"""

alias ElixirAnalyzer.ExerciseTest.CommonChecks.FunctionNames
alias ElixirAnalyzer.ExerciseTest.CommonChecks.ModuleAttributeNames
alias ElixirAnalyzer.Comment

@spec run(Macro.t(), String.t()) :: [{:pass | :fail | :skip, %Comment{}}]
def run(code_ast, code_as_string) when is_binary(code_as_string) do
[
FunctionNames.run(code_ast),
ModuleAttributeNames.run(code_ast)
]
|> List.flatten()
end
end
63 changes: 63 additions & 0 deletions lib/elixir_analyzer/exercise_test/common_checks/function_names.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.FunctionNames do
@moduledoc """
Reports the first function/macro/guard with a name that's not snake_case.

Doesn't report more if there are more.
A single comment should be enough for the student to know what to fix.

Common check to be run on every single solution.
"""

alias ElixirAnalyzer.Constants
alias ElixirAnalyzer.Comment

@def_ops [:def, :defp, :defmacro, :defmacrop, :defguard, :defguardp]

@spec run(Macro.t()) :: [{:pass | :fail | :skip, %Comment{}}]
def run(ast) do
{_, names} = Macro.prewalk(ast, [], &traverse/2)
wrong_name = List.last(names)

if wrong_name do
wrong_name = to_string(wrong_name)
correct_name = to_snake_case(wrong_name)

[
{:fail,
%Comment{
type: :actionable,
comment: Constants.solution_function_name_snake_case(),
params: %{
expected: correct_name,
actual: wrong_name
}
}}
]
else
[]
end
end

defp traverse({op, _meta, [{name, _meta2, _arguments} | _]} = ast, names) when op in @def_ops do
if snake_case?(name) do
{ast, names}
else
{ast, [name | names]}
end
end

defp traverse(ast, names) do
{ast, names}
end

defp snake_case?(name) do
# the code had to compile and pass all the tests to get to the analyzer
# so we can assume the name is otherwise valid
to_snake_case(name) == to_string(name)
end

defp to_snake_case(name) do
# Macro.underscore is good enough because a module attribute name must be a valid Elixir identifier anyway
Copy link
Contributor

Choose a reason for hiding this comment

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

100% agree, despite the warning on the function not to use, I see it used everywhere in non-core code :P

Macro.underscore(to_string(name))
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
defmodule ElixirAnalyzer.ExerciseTest.CommonChecks.ModuleAttributeNames do
@moduledoc """
Reports the first module attribute with a name that's not snake_case.

Doesn't report more if there are more.
A single comment should be enough for the student to know what to fix.

Common check to be run on every single solution.
"""

alias ElixirAnalyzer.Constants
alias ElixirAnalyzer.Comment

@spec run(Macro.t()) :: [{:pass | :fail | :skip, %Comment{}}]
def run(ast) do
{_, names} = Macro.prewalk(ast, [], &traverse/2)
wrong_name = List.last(names)

if wrong_name do
wrong_name = to_string(wrong_name)
correct_name = to_snake_case(wrong_name)

[
{:fail,
%Comment{
type: :actionable,
comment: Constants.solution_module_attribute_name_snake_case(),
params: %{
expected: correct_name,
actual: wrong_name
}
}}
]
else
[]
end
end

defp traverse({:@, _meta, [{name, _meta2, _arguments}]} = ast, names) do
if snake_case?(name) do
{ast, names}
else
{ast, [name | names]}
end
end

defp traverse(ast, names) do
{ast, names}
end

defp snake_case?(name) do
# the code had to compile and pass all the tests to get to the analyzer
# so we can assume the name is otherwise valid
to_snake_case(name) == to_string(name)
end

defp to_snake_case(name) do
# Macro.underscore is good enough because a module attribute name must be a valid Elixir identifier anyway
Macro.underscore(to_string(name))
end
end
10 changes: 10 additions & 0 deletions lib/elixir_analyzer/test_suite/default.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
defmodule ElixirAnalyzer.TestSuite.Default do
@moduledoc """
This is the default exercise analyzer extension module.

It will be run for any exercise submission that doesn't have its own extension module.
It's intentionally empty, which means it will only run the common checks.
"""

use ElixirAnalyzer.ExerciseTest
end
Loading