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

Bugfix: ParseCFlagsFromMakeOutput was never being called #3574

Merged
merged 1 commit into from
Feb 6, 2021

Conversation

hugomg
Copy link
Contributor

@hugomg hugomg commented Feb 3, 2021

Previously, the make output was never being parsed, even when the
c_parse_makefile option was set. I think this is because of a bug in GetCFlags, which would only try parsing the makefile if the compile_commands.json was ALSO present.

I'm not sure if I need to add tests for this, because it's a small change. If it is then I can come back later to add those.

Previously, the make output was never being parsed, even when the
`c_parse_makefile` option was set.
Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

I am not familiar with this part of ALE but the fact that there are many tests for Makefile flags parsing (see test_c_flag_parsing.vader) and they were passing before this fix makes me suspect something is wrong. I would recommend to check the tests and see why they worked before and after this fix.

@hugomg
Copy link
Contributor Author

hugomg commented Feb 4, 2021

On the matter of test_c_flag_parsing, my impression is that the tests that we have are unit tests to see if the ParseCFlagsFromMakeOutput function does the right thing. However, the bug that I found was that ParseCFlagsFromMakeOutput wasn't being called in the first place...

It's possible that there really are no tests that could have caught this. I might be missing something but I can't find any tests that call ale_linters#c#cc#GetCommand or ale#c#GetCFlags.

By the way, according to the git log the commit that introduced the contentious if statement appears to be this one: affeed7

@hsanson hsanson requested a review from w0rp February 5, 2021 14:00
@hsanson
Copy link
Contributor

hsanson commented Feb 5, 2021

I would recommend this PR be reviewed by @w0rp.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Yeah, this is just a mistake I made. I wouldn't worry about unit testing every single thing in the universe, I'll just merge this.

@w0rp w0rp merged commit 77c0348 into dense-analysis:master Feb 6, 2021
@w0rp
Copy link
Member

w0rp commented Feb 6, 2021

Cheers! 🍻

@hugomg hugomg deleted the parse_makefile branch February 6, 2021 19:47
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.

None yet

3 participants