Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
100% test coverage #246
100% test coverage #246
Changes from all commits
05ac98f
c7464a5
f537273
541319f
367afaa
d6d38e6
2a928a6
c5ff3d9
d6fc181
08dc424
74063df
7284304
d15d73d
ff039e1
65bbe0d
ae496ca
11c53f9
db5858f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not understand these options here, all they do is overwrite the required arguments. Those options were passed from the CLI module. I got rid of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice getting rid of a duplicated argument 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the purpose of skipping analysis except for debugging, so I got rid of that option too. I think it still exists somewhere in
feature
though, although it's never used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the ability to skip a single check? Because I don't see anything related to skipping analysis as a whole anywhere else 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I suppose that's what I meant. There's probably no relation beyond the name.
Anyway, I don't see the point of skipping anything, just remove the code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an option in the CLI, but could not actually be passed to the analyzer, so I got rid of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you discover this bug? When I bring it back, no tests fail, so it wasn't by writing tests 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that! I took me a while to remember.
I think it was from this test:
elixir-analyzer/test/elixir_analyzer/exercise_test/common_checks/function_capture_test.exs
Lines 53 to 58 in 926d69d
At some point I was checking if
actual_function?
gave the right result, I was printing some values and I was expecting to see that test case pop out, but it never came, so I realized thatdepth
was becoming negative.I've added a failing test now. Let that be a valuable lesson of never fixing bugs without context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🎉 I didn't notice before that this one macro is missing the comment check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit hesitant to get rid of this. It seems like it could be useful, but it's not actually used anywhere. I'm assuming at this point we won't need to add another complex analysis tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No mercy for dead code! Even if it looks potentially useful, we will be able to recreate it when we need it. Making the code base easier to understand by removing trash is the better option every time, IMO.