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

Warn people that Haml 5 is not fully supported #1379

Merged
merged 1 commit into from Jul 12, 2019

Conversation

@jaredbeck
Copy link
Contributor

commented Jul 10, 2019

[Fixes #1370]

I don't have great confidence this is the best implementation, and I'd appreciate feedback.

  • How should I write a test for this? Which test file should it go in?
  • In my test, how do I assert that a Brakeman.notify occurred?

Thanks.

@jaredbeck

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

The fact that the implementation is messy doesn't bother me too much, because we can delete it all when we add support for Haml 5. (Of course, if it bothers you, please say so) My only real concern so far is that I couldn't figure out how to test it.

@presidentbeef
Copy link
Owner

left a comment

How should I write a test for this? Which test file should it go in?
In my test, how do I assert that a Brakeman.notify occurred?

You can add it to test/tests/brakeman.rb. A bit of a catchall but it has similar tests. I'm not too picky.

You should be able to use assert_output and test that the message went to stderr. There are other examples in the test file.

For this case, you can either test a Brakeman.run on the test app, or you could add tests specifically for Tracker::Config#check_haml_version and Tracker::Config#supported_haml_versionl?.

# known Haml 5 versions and test whether the `Requirement` is satisfied. I
# included '5.99.99' in the list so that this method might stand a chance of
# working in the future.
def supported_haml_version?

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jul 10, 2019

Owner

In many cases, there will be an exact gem version from Gemfile.lock.

I am okay with just using version_between? which will handle most cases.

This comment has been minimized.

Copy link
@jaredbeck

jaredbeck Jul 10, 2019

Author Contributor

version_between? expects a single version number, but all we have is a "requirement", right? Consider the requirement > 4.99. This requirement will install Haml 5. It would be incorrect to ignore the operator and only consider the number, 4.99. version_between? would think it is Haml 4.

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jul 10, 2019

Owner

That is an edge case, yes, but again most applications are going to have a lock file with the exact version, not just the requirement.

CHANGES.md Outdated
@@ -6,6 +6,7 @@
* Avoid assigning `nil` line numbers to `Sexp`s
* Add special warning code for custom checks
* Add call matching by regular expression
# Warn people that Haml 5 is not fully supported

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jul 10, 2019

Owner

Should be * not #

@@ -111,6 +111,8 @@ def set_rails_version
@escape_html = true
Brakeman.notify "[Notice] Escaping HTML by default"
end

check_haml_version

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jul 10, 2019

Owner

Kind of a weird place for this...but I don't know if there is a less weird place.

@jaredbeck jaredbeck force-pushed the jaredbeck:fix_issue_1370 branch from 810795d to 66e0c0a Jul 10, 2019

@jaredbeck

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

You can add it to test/tests/brakeman.rb.

Test added, thanks for the guidance!

:app_path => "#{TEST_PATH}/apps/rails5.2",
:run_checks => []
}
_out, err = capture_io { Brakeman.run options }

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Jul 11, 2019

Owner

No need to generate a config file here.

@presidentbeef presidentbeef merged commit 2d8f980 into presidentbeef:master Jul 12, 2019

9 checks passed

ci/circleci: default Your tests passed on CircleCI!
Details
ci/circleci: test-2-3 Your tests passed on CircleCI!
Details
ci/circleci: test-2-4 Your tests passed on CircleCI!
Details
ci/circleci: test-2-5 Your tests passed on CircleCI!
Details
ci/circleci: test-2-6 Your tests passed on CircleCI!
Details
ci/circleci: upload-coverage Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/diff-coverage 100% (98% threshold)
Details
codeclimate/total-coverage 95% (0.0% change)
Details

@jaredbeck jaredbeck deleted the jaredbeck:fix_issue_1370 branch Jul 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.