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.

Sign up for GitHub

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

Closes #398 - Fixed newsletter analysis to allow guard clause #400

Merged
merged 4 commits into from
Oct 13, 2023
Merged

Closes #398 - Fixed newsletter analysis to allow guard clause #400

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83fda08
Fixed newsletter analysis to allow guard clause
octowombat Oct 13, 2023
2035413
Reverted removal of compiler warning
octowombat Oct 13, 2023
751f454
Added other variants to suport guard clause
octowombat Oct 13, 2023
1045672
Added other variant tests with guard clause
octowombat Oct 13, 2023
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
40 changes: 40 additions & 0 deletions lib/elixir_analyzer/test_suite/newsletter.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ defmodule ElixirAnalyzer.TestSuite.Newsletter do
end
end
end

form do
def close_log(_ignore) when _ignore do
_block_ends_with do
:ok
end
end
end
end

feature "use IO.puts in log_sent_email rather than IO.write as last call" do
Expand All @@ -33,6 +41,14 @@ defmodule ElixirAnalyzer.TestSuite.Newsletter do
end
end
end

form do
def log_sent_email(_ignore, _ignore) when _ignore do
_block_ends_with do
:ok
end
end
end
end

assert_no_call "use IO.puts in log_sent_email rather than IO.write" do
Expand All @@ -55,6 +71,14 @@ defmodule ElixirAnalyzer.TestSuite.Newsletter do
end
end
end

form do
def log_sent_email(_ignore, _ignore) when _ignore do
_block_ends_with do
:ok
end
end
end
end

feature "send_newsletter ends with close_log" do
Expand All @@ -69,6 +93,14 @@ defmodule ElixirAnalyzer.TestSuite.Newsletter do
end
end
end

form do
def send_newsletter(_ignore, _ignore, _ignore) when _ignore do
_block_ends_with do
:ok
end
end
end
end

feature "open_log used the option :write" do
Expand Down Expand Up @@ -107,6 +139,14 @@ defmodule ElixirAnalyzer.TestSuite.Newsletter do
end
end
end

form do
def open_log(_ignore) when _ignore do
_block_includes do
File.open!(_ignore, [:write])
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also include all of the variations we had before (with and without File, with and without !). I know they are not all super likely, but we might as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't remove any of the existing variants though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant let's duplicate all four existing variants and add the when _ignore, not just for File.open!.

Right now if someone uses

        import File
        def open_log(path) when is_binary(path) do
          open!(path, [:write])
        end

It will trigger a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha!

end
end
end
end

assert_call "send_newsletter/3 calls open_log/1" do
Expand Down
34 changes: 34 additions & 0 deletions test/elixir_analyzer/test_suite/newsletter_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,40 @@ defmodule ElixirAnalyzer.ExerciseTest.NewsletterTest do
if f.(email) === :ok, do: log_sent_email(log_pid, email)
do_send(rest, log_pid, f)
end
end,
defmodule Newsletter do
def read_emails(path) do
path
|> File.read!()
|> String.split()
end

def open_log(path) when is_binary(path) do
File.open!(path, [:write])
end

def log_sent_email(pid, email) when is_pid(pid) and is_binary(email) do
IO.puts(pid, email)
end

def close_log(pid) when is_pid(pid) do
File.close(pid)
end

def send_newsletter(emails_path, log_path, send_fun)
when is_binary(emails_path) and is_binary(log_path) and is_function(send_fun, 1) do
log_pid = open_log(log_path)
emails = read_emails(emails_path)

Enum.each(emails, fn email ->
case send_fun.(email) do
:ok -> log_sent_email(log_pid, email)
_ -> nil
end
end)

close_log(log_pid)
end
end
]
end
Expand Down
1 change: 0 additions & 1 deletion test_data/two_fer/imperfect_solution/lib/two_fer.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
defmodule TwoFer do
@someUnusedModuleAttribute 1
Copy link
Contributor

@jiegillet jiegillet Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Did you change this by mistake?
This is the source of the failing tests.

Copy link
Contributor Author

@octowombat octowombat Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I did change it as it caused an unused compiler warning and my setup locally stops in that case!!

I will revert the change and retest. Thanks.

Yes that fixes the tests! I guess I don't know the codebase well enough (yet!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, the compiler warning is on purpose, so we can test if we actually detect it :)


defmodule My_empty_module do
end
Expand Down
Loading