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

Fix Credo.Check.Refactor.Apply #897

Merged
merged 5 commits into from
Oct 30, 2021
Merged

Fix Credo.Check.Refactor.Apply #897

merged 5 commits into from
Oct 30, 2021

Conversation

NickNeck
Copy link
Contributor

@NickNeck NickNeck commented Oct 1, 2021

closes #892

Comment on lines 58 to 62
test "it should report a violation for apply/2" do
"""
defmodule Test do
def some_function(fun, arg1, arg2) do
apply(fun, [arg1, arg2])
def some_function(arg1, arg2) do
apply(:fun_name, [arg1, arg2])
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need to update this particular test in a PR fixing #892?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new test above replaced these tests, and this test is now one of the two tests for testing that the check reports a violation. The check reports now just a violation when fun_name is an atom and args is a list. The mentioned test is testing this now for apply/2.

Copy link
Owner

Choose a reason for hiding this comment

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

But there is no longer a test ensuring that apply/2 works with a variable given for the fun name? Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, now I see. I will add some more tests for apply/2.

@rrrene rrrene merged commit b876934 into rrrene:master Oct 30, 2021
@rrrene
Copy link
Owner

rrrene commented Oct 30, 2021

@NickNeck Thx! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Credo.Check.Refactor.Apply shouldn't apply if the function name is sourced from a variable
2 participants