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

Allow method to be specified by regex #710

Merged
merged 1 commit into from Sep 5, 2015

Conversation

aianus
Copy link
Contributor

@aianus aianus commented Sep 1, 2015

The options to find_calls includes :method - symbol, array of symbols, or regular expression to match method(s) but passing a regular expression without a target does not actually work without this patch.

@presidentbeef
Copy link
Owner

Hi Alex,

Yes, it was removed for performance reasons: http://blog.presidentbeef.com/blog/2012/11/28/faster-call-indexing-in-brakeman-1-dot-8-3/

Do you need this functionality? If so we'll have to look into better performing code. Right now it would convert every single call to a string every time it tried to match a regex. This could make it even slower than it was before.

@aianus
Copy link
Contributor Author

aianus commented Sep 1, 2015

Yes, we are using this functionality.

Seems to me it should be up to the author and user of the check if they want to use this slower search method. For our use case at Coinbase searching by regex was plenty fast and our app is pretty 'large'.

@presidentbeef
Copy link
Owner

It's possible to support it and I'm happy to do so if it's something you are using. The trouble previously was converting the target to strings, while this change is only doing methods which is considerably cheaper.

presidentbeef added a commit that referenced this pull request Sep 5, 2015
Allow method to be specified by regex
@presidentbeef presidentbeef merged commit f9ce050 into presidentbeef:master Sep 5, 2015
Repository owner locked and limited conversation to collaborators Feb 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants