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

Ignore Tempfiles in FileAccess warnings #1244

Merged
merged 1 commit into from Aug 13, 2018

Conversation

Projects
None yet
3 participants
@cmkoller
Copy link
Contributor

cmkoller commented Jul 18, 2018

Fixes #1243

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Jul 20, 2018

Hi Christina,

Thank you for the PR! Please acknowledge that copyright for all contributions will be assigned to Synopsys, Inc. per our contribution guidelines.

The approach seems a little bit broad - what about just checking the file_name? Preferably something like call? file_name and file_name.target == s(:const, :Tempfile).

@cmkoller

This comment has been minimized.

Copy link
Contributor Author

cmkoller commented Jul 24, 2018

Hey @presidentbeef, thanks for the comment! Would you mind explaining a little further what you're picturing? I'm totally with you that just checking for Tempfile feels a little broad! But I'm not quite sure how to check for file_name like you're suggesting... I don't think that there's a file_name method in the call chain here, although maybe I'm misunderstanding.

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Jul 25, 2018

Right now you are checking the entire call, but just a couple lines down there's the file_name (which is just the first argument to the call). So you can check that instead. I assume we want to ignore any calls on Tempfile, so checking that file_name is a call expression and checking that the target is the Tempfile constant should be enough. That's the code I showed above.

There are a number of convenience methods like call? in Util and some convenience methods on Sexp in here.

Let me know if that doesn't make sense.

@cmkoller cmkoller force-pushed the cmkoller:ignore-tempfile-file-access-warnings branch from 842d25d to 2dc5e44 Jul 26, 2018

@cmkoller

This comment has been minimized.

Copy link
Contributor Author

cmkoller commented Jul 26, 2018

@presidentbeef Thanks for clarifying! I pushed an update using your suggestion :) let me know what you think!

@JacobEvelyn

This comment has been minimized.

Copy link
Contributor

JacobEvelyn commented Aug 10, 2018

Just wondering if there's any reason for the delay here? This change would be really nice!

@presidentbeef presidentbeef merged commit af9d7cb into presidentbeef:master Aug 13, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/diff-coverage 100% (90% threshold)
Details
codeclimate/total-coverage 94% (0.0% change)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Aug 13, 2018

Just wondering if there's any reason for the delay here? This change would be really nice!

Nope, just life...

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Aug 13, 2018

Thanks @cmkoller!

Repository owner locked and limited conversation to collaborators Oct 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.