Add new `check_source` check to check source code as a string #189

jiegillet · 2021-10-09T14:09:54Z

Closes part 2 of #169.
This one came together faster than expected.
It might be easiest to review commit by commit.

If it gets approved, I will also add it the docs and recipe (with warnings to only use this if other techniques can't work).

neenjaw

Nice addition, I have a few questions/comments that I think should be addressed before merge.

lib/elixir_analyzer/exercise_test/check_source.ex

neenjaw · 2021-10-11T03:20:15Z

lib/elixir_analyzer/exercise_test/check_source.ex

+    {node, Map.put(test_data, :comment, comment)}
+  end
+
+  @supported_types ~w(essential actionable informative celebratory)a


Either in this pr or another, this check for the supported check levels is probably something that is repeated. Can we extract this check to another module so we can just do something like this:

# ... if not AnalyzerCaseUtil.supported_case_type?(type) do raise #... end # ...

If you want to do this in another PR (which I think is fine and keeps this PR's focus singular), can you open an issue and assign to me? I would like to do this if not already done, thanks.

I've thought about that actually, some code is now duplicated three times, which starts to smell like a refactor. I'd be happy to assign this to you, once this gets merged.

Maybe even

defp do_walk_check_source_block({:type, _, [type]} = node, test_data) do AnalyzerCaseUtil.supported_case_type?(type, :check_source)

If it works well for the other cases.

Pros and cons for sure. There is a cost to drying up code too much. If you have the bandwidth, reading 99 bottles of oop is a good discussion on this.

Passing the type of check (:check_source) creates a dependency in that future Util function because it then has to "know" about all of the types of check that exist which perhaps all it really need to know is if the type is one of the standard 4 types.

I was just thinking of inserting :check_source in the string raised, but I do see your point.

test/elixir_analyzer/exercise_test/check_source_test.exs

Co-authored-by: Tim Austin <tim@neenjaw.com>

lib/elixir_analyzer/exercise_test.ex

angelikatyborska · 2021-10-11T18:50:23Z

lib/elixir_analyzer/test_suite/german_sysadmin.ex

+    type :actionable
+    comment Constants.german_sysadmin_no_integer_literal()
+
+    check(source) do


String source only? No AST?

Yeah... Originally I had both, so it's not like it's impossible. I removed it because it felt too complex, somehow. And since the AST is built from the string to begin with, it's very simple to use Code.string_to_quoted to get the AST.

I'm on the fence, so if you think it's better design, I will put it back in. The test I would add would probably be the same as the indentation check, that's the best use I can think of.

I can think of many checks that I would want to do on the AST that current macros do not allow, but you're right, we can just call Code.string_to_quoted. As long as it doesn't happen a thousand times in a single analyzer run, there's probably no need to worry about the performance.

OK. And if end up using ASTs many times, it won't be difficult to put in back in.

angelikatyborska · 2021-10-11T19:03:16Z

lib/elixir_analyzer/test_suite/german_sysadmin.ex

+      integers = ["252", "246", "228", "223"]
+      not Enum.any?(integers, &String.contains?(source, &1))


I'm worried that this check is a bit weak. It could be accidentally triggered by something silly in a comment, for example writing ?ü # 252 😁. What if we did the opposite check - ensure that the strings "?ü" etc are all in the source code? Maybe for all the special values from the exercise, so including a, z, and underscore. I just saw a solution that only used the question mark syntax for the special characters, but not the simple ones.

It's not much better, but at least it won't be falsely triggered by comments.

Sounds good, it's a more robust check.

test/elixir_analyzer/test_suite/german_sysadmin_test.exs

Co-authored-by: Angelika Tyborska <angelikatyborska@fastmail.com>

lib/elixir_analyzer/exercise_test.ex

angelikatyborska

LGTM (except for the remaining typos and comment that still needs merging)

It will have false-positives for some solutions that I can see in community solutions right now. They're all all over the place... using Enum.filter when it's completely unnecessary or spelling out the whole alphabet as a charlist 🤷. But I think it's better to have those false positives now and help all of those people lost without ? and add more analyzer check for the exercise later.

angelikatyborska · 2021-10-13T15:50:27Z

test/support/analyzer_verification/check_source.ex

+    end
+  end
+
+  check_source "finds integer litteral" do


I just noticed that this typo with double t is everywhere...

I'm so sorry, its the French spelling. My English is pretty good, but I have a few words that I simply cannot spell right (exercice, litterature, comparaison, ...)

Ah, that explains it! I have words that I can't spell correctly either. Having a dictionary in the text editor helps a lot. That's how I even noticed those typos 😁

angelikatyborska · 2021-10-13T15:51:08Z

test/elixir_analyzer/exercise_test/check_source_test.exs

+        case game do
+          "fifa" -> "fifa 98"
+          "tomb raider" -> "tomb raider II (1997)"
+          _ -> "goat simulator"


Hard to argue with that code 👍

angelikatyborska · 2021-10-14T07:06:44Z

Let's merge! I'm waiting on the docs and recipes update in a new PR :)

jiegillet added 5 commits October 9, 2021 23:02

Update submodule

08ebf98

Add new check_source check

81b1b53

Add integer literal test for german-sysadmin

0868950

Add unit tests for check_source

7fbc051

Complete suppress_if tests

c74f81f

jiegillet added x:action/create Work on something from scratch x:module/analyzer Work on Analyzers x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) x:size/large Large amount of work labels Oct 9, 2021

jiegillet requested a review from angelikatyborska as a code owner October 9, 2021 14:09

neenjaw requested changes Oct 11, 2021

View reviewed changes

Typo

1928e45

Co-authored-by: Tim Austin <tim@neenjaw.com>

jiegillet added the hacktoberfest-accepted label Oct 11, 2021

neenjaw approved these changes Oct 11, 2021

View reviewed changes

angelikatyborska reviewed Oct 11, 2021

View reviewed changes

Typo

79accdf

Co-authored-by: Angelika Tyborska <angelikatyborska@fastmail.com>

jiegillet commented Oct 12, 2021

View reviewed changes

lib/elixir_analyzer/exercise_test.ex Outdated Show resolved Hide resolved

jiegillet added 4 commits October 12, 2021 10:07

Copy-paste + forgot to update comment

f8ca3a5

Change check to detect ?ß rather than not 223

4d3e1b8

Remove file added by mistake

b30462f

Move integer literal comment to solution for general use

baf9f7b

angelikatyborska approved these changes Oct 13, 2021

View reviewed changes

angelikatyborska mentioned this pull request Oct 13, 2021

Add common check for removing boilerplate comments #195

Closed

literal typos

e4dcd70

angelikatyborska merged commit f3e1868 into exercism:main Oct 14, 2021

jiegillet deleted the jie-check-source branch October 14, 2021 08:07

This was referenced Oct 14, 2021

Write analysis for concept exercise high-school-sweetheart #86

Closed

Refactor supported check types from feature, assert_call and check_source #196

Closed

Handle common problems in german-sysadmin #169

Closed

This was referenced Jul 14, 2022

Document reputation label exercism/docs#347

Merged

Consider correct reputation amounts for different labels exercism/exercism#6440

Open

Consider correct reputation amounts for different labels exercism/exercism#6441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `check_source` check to check source code as a string #189

Add new `check_source` check to check source code as a string #189

jiegillet commented Oct 9, 2021 •

edited

Loading

neenjaw left a comment

neenjaw Oct 11, 2021

neenjaw Oct 11, 2021

jiegillet Oct 11, 2021

neenjaw Oct 11, 2021 •

edited

Loading

jiegillet Oct 11, 2021

angelikatyborska Oct 11, 2021

jiegillet Oct 12, 2021

angelikatyborska Oct 12, 2021

jiegillet Oct 12, 2021

angelikatyborska Oct 11, 2021

jiegillet Oct 12, 2021

angelikatyborska left a comment •

edited

Loading

angelikatyborska Oct 13, 2021

jiegillet Oct 14, 2021

angelikatyborska Oct 14, 2021

angelikatyborska Oct 13, 2021

angelikatyborska commented Oct 14, 2021

		integers = ["252", "246", "228", "223"]
		not Enum.any?(integers, &String.contains?(source, &1))

Add new check_source check to check source code as a string #189

Add new check_source check to check source code as a string #189

Conversation

jiegillet commented Oct 9, 2021 • edited Loading

neenjaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neenjaw Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelikatyborska left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelikatyborska commented Oct 14, 2021

Add new `check_source` check to check source code as a string #189

Add new `check_source` check to check source code as a string #189

jiegillet commented Oct 9, 2021 •

edited

Loading

neenjaw Oct 11, 2021 •

edited

Loading

angelikatyborska left a comment •

edited

Loading