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

fix NoMethodError exception using --compare flag #1024

Merged
merged 1 commit into from Mar 23, 2017

Conversation

Projects
None yet
3 participants
@seangransee
Contributor

seangransee commented Mar 23, 2017

Fixes #1023

In brakeman#L95, tracker is used twice outside the block in which it's defined. I think you intended to have this conditional block inside the else block immediately above it.

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Mar 23, 2017

Owner

Thanks!

I think this needs to be in both branches... @grosser ?

Owner

presidentbeef commented Mar 23, 2017

Thanks!

I think this needs to be in both branches... @grosser ?

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Mar 23, 2017

Contributor

using compare and exit-on-error does not work together ... how about changing tracker.options[:exit_on_error] -> options[:exit_on_error] ... so it still blows up when used together (instead of silently ignoring it) but does not blow up when just using compare ?

Contributor

grosser commented Mar 23, 2017

using compare and exit-on-error does not work together ... how about changing tracker.options[:exit_on_error] -> options[:exit_on_error] ... so it still blows up when used together (instead of silently ignoring it) but does not blow up when just using compare ?

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Mar 23, 2017

Owner

Why can't they work together? --compare and --exit-on-warn do.

Owner

presidentbeef commented Mar 23, 2017

Why can't they work together? --compare and --exit-on-warn do.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Mar 23, 2017

Contributor

they can, just needs more work .. is not a trivial bugfix

Contributor

grosser commented Mar 23, 2017

they can, just needs more work .. is not a trivial bugfix

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Mar 23, 2017

Owner

🤔 copying the condition to both branches doesn't work? Not sure why that would be complicated.

Owner

presidentbeef commented Mar 23, 2017

🤔 copying the condition to both branches doesn't work? Not sure why that would be complicated.

@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser Mar 23, 2017

Contributor

it uses the tracker which the other branch does not have

Contributor

grosser commented Mar 23, 2017

it uses the tracker which the other branch does not have

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Mar 23, 2017

Owner

AH okay. Then I'm fine with this.

Owner

presidentbeef commented Mar 23, 2017

AH okay. Then I'm fine with this.

@presidentbeef presidentbeef merged commit 74930fc into presidentbeef:master Mar 23, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Repository owner locked and limited conversation to collaborators May 18, 2017

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