-
Notifications
You must be signed in to change notification settings - Fork 3
Create Test case for config files #103
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
Conversation
43dfb62 to
d408c79
Compare
Szelethus
left a comment
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.
The patch overall looks OK, but can you beef up the comments a bit? Something like this:
Set to true -> Set to true after fix; should've copied the input config file to bazel-bin
Set to 0 -> Set to 0 after fix; the config file should've explicitly disabled the division-by-zero checker, but the file is unrecognized by per-file analysis
Also, I'm not sure how important it is to check the first lines of the original/copied config files.
|
I'm pretty sure that I'm comparing the whole file contents, not just the first line. |
Szelethus
left a comment
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.
LGTM! I won't annoy you to death with my nits (we will delete them in a followup patch anyways), so I'll leave it to you how much you want to fancy them up. The nits were mainly meant as a general advice on how these comments would be ideal if you were to write a similar patch in the future.
Why:
We want to support config files; we should test for them.
What:
Addresses:
#30
Test for #102