Skip to content

Fix timed geocoder with exception flow #376

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

Merged
merged 1 commit into from
Dec 8, 2014
Merged

Fix timed geocoder with exception flow #376

merged 1 commit into from
Dec 8, 2014

Conversation

egeloen
Copy link
Contributor

@egeloen egeloen commented Dec 7, 2014

All is in the title. Before this PR, the timed geocoder didn't stop the stopwatch component when a exception was thrown.

}

public function testGeocode()
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was indented using 3 spaces instead of 4. I fixed it too.

@Baachi
Copy link
Member

Baachi commented Dec 7, 2014

Is this really needed? If an exception occured, the script should stop and call the error handler. The TimedTwigEngine have nearly the same code.

@egeloen
Copy link
Contributor Author

egeloen commented Dec 7, 2014

Don't really know if it is needed. Personally, I have done the same here.

It just seems strange the stopwatch is not "really" stopped when an exception is thrown whereas you want it to be stopped. Basically, this is not because there is an exception thrown then your application must stop to run. You can catch it and continue your script.

@Baachi
Copy link
Member

Baachi commented Dec 7, 2014

I know, but now you have an wrong result. Let's say the provider isn't reachable, and the timeout from 10 seconds is reached. Now you have an entry in your timeline which says, the provider takes 10 seconds to fetch the result. I dont't think this is the right way to go.

@egeloen
Copy link
Contributor Author

egeloen commented Dec 7, 2014

Personally, I use the stopwatch component to time good and errored requests. If my script takes 15 seconds and Geocoder takes 10 seconds, I would expect to have it timed right regardless if the request have good or not. Technically, Geocoder has been used during 10 seconds and so, I would expect it to be displayed as it in all cases...

@Baachi
Copy link
Member

Baachi commented Dec 7, 2014

Maybe my example wasn't good enough. But i'm not against this PR, if it help some people, i'm okay with this small change.

willdurand added a commit that referenced this pull request Dec 8, 2014
Fix timed geocoder with exception flow
@willdurand willdurand merged commit 86afdd9 into geocoder-php:master Dec 8, 2014
@egeloen egeloen deleted the timed-geocoder-exception branch December 8, 2014 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants