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

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

Merged
merged 4 commits into from
Oct 13, 2023

Conversation

octowombat
Copy link
Contributor

Following discussions with @jiegillet and an upstream code fix.

@@ -1,5 +1,4 @@
defmodule TwoFer do
@someUnusedModuleAttribute 1
Copy link
Contributor

@jiegillet jiegillet Oct 13, 2023

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

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 :)

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!

@jiegillet jiegillet self-assigned this Oct 13, 2023
@jiegillet
Copy link
Contributor

jiegillet commented Oct 13, 2023

Sorry one last thing: for completeness, we should add tests for all 8 cases, something like

  test_exercise_analysis "open_log accepts 8 several variants",
    comments_exclude: [ElixirAnalyzer.Constants.newsletter_open_log_uses_option_write()] do
   [ defmodule Newsletter do
        import File
        def open_log(path) when is_binary(path) do
          open!(path, [:write])
        end
    end
  end,
   ....
]

Copy link
Contributor

@jiegillet jiegillet left a comment

Choose a reason for hiding this comment

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

That's great, thank you so much for your contribution!

@jiegillet jiegillet merged commit 02c3574 into exercism:main Oct 13, 2023
3 checks passed
@octowombat
Copy link
Contributor Author

My pleasure

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.

2 participants