yegor256 / sixnines

Website Availability Monitor: add your website to our dashboard and get 24x7 monitoring of its availability (and a badge!)
https://www.sixnines.io
MIT License
72 stars 10 forks source link

Bugfix/search #78

Closed pdacostaporto closed 7 years ago

pdacostaporto commented 7 years ago

fixes #75

0crat commented 7 years ago

@yegor256 please, pay attention to this pull request

0crat commented 7 years ago

Job gh:yegor256/sixnines#78 is in scope.

0crat commented 7 years ago

+15 points just awarded to @pdacostaporto, total is +15.

codecov-io commented 7 years ago

Codecov Report

Merging #78 into master will decrease coverage by 0.26%. The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   40.39%   40.13%   -0.27%     
==========================================
  Files          19       19              
  Lines         604      613       +9     
==========================================
+ Hits          244      246       +2     
- Misses        360      367       +7
Impacted Files Coverage Δ
objects/endpoint.rb 34.24% <0%> (ø) :arrow_up:
test/test_endpoint.rb 32.35% <20%> (-2.13%) :arrow_down:
test/test_sixnines.rb 33.7% <25%> (-0.41%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 154af49...5af0956. Read the comment docs.

yegor256 commented 7 years ago

@pdacostaporto many thanks, see my comments above!

pdacostaporto commented 7 years ago

@yegor256 I implemented it the way you said. Should I close this PR and open a new one?

yegor256 commented 7 years ago

@pdacostaporto it's not the null problem here. Well, it is also, but the main trouble is that the view script doesn't check whether the value exists in the map and just retrieves it from there. And the map returns nil instead of throwing an exception. The right way would be to check first. Instead of filling the map with some placeholders.

pdacostaporto commented 7 years ago

@yegor256 I made a new PR (#79) with the approach you said. (Was it OK to make a new one?) I guess this one should be closed.

yegor256 commented 7 years ago

@pdacostaporto yes, correct, just close this one please

0crat commented 7 years ago

Job gh:yegor256/sixnines#78 is now out of scope.