throw specific HTTPStatusException on http request errors by MartinNowak · Pull Request #5551 · dlang/phobos

MartinNowak · 2017-07-05T19:11:41Z

To make them distinguishable from (e.g. connection) errors and allow
appropriate handling in client code.

dlang-bot · 2017-07-05T19:11:42Z

Thanks for your pull request, @MartinNowak!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

- also see dlang/phobos#5551 for HTTPStatusException

wilzbach · 2017-07-06T01:18:36Z

std/net/curl.d

-                            format("HTTP request returned status code %d (%s)",
-                                   statusLine.code, statusLine.reason));
+    enforce(statusLine.code / 100 == 2, new HTTPStatusException(statusLine.code,
+            format("HTTP request returned status code %d (%s)", statusLine.code, statusLine.reason)));


This message seems to be rather generic - what was the motivation for not generating this message in HTTPStatusException (or providing a second overload)?

It's the baseline of a useful error message but doesn't contain enough information on it's own (e.g. the url or the status reason), those are only available by the calling code.

Yes that was my point - if no specific message is passed, this could be the baseline.

So... not sure what the proposal is here.

Sth. like below could be handy, but I don't feel strongly about this (I am not using std.net.curl anyways):

@safe pure nothrow this(int status, string file = __FILE__, size_t line = __LINE__, Throwable next = null) { const msg = format("HTTP request returned status code %d", status); super(msg, file, line, next); this.status = status; } @safe pure nothrow this(int status, string msg, string file = __FILE__, size_t line = __LINE__, Throwable next = null) { const _msg = format("HTTP request returned status code %d (%s)", status, msg); super(_msg, file, line, next); this.status = status; }

PetarKirov · 2017-07-06T14:18:51Z

std/net/curl.d

+        super(msg, file, line, next);
+    }
+
+    int status; /// The HTTP status code


Make that const or immutable?

- also see dlang/phobos#5551 for HTTPStatusException

wilzbach · 2017-07-08T06:59:27Z

@ZombineDev there is still the rule that new symbols should be approved by @andralex (we should really document these and make a handy guide out of them)

andralex · 2017-07-08T11:21:49Z

std/net/curl.d

+            next = The previous exception in the chain of exceptions, if any.
+      +/
+    @safe pure nothrow
+        this(


this line should be flush with the attributes, i.e.

@safe pure nothrow this(

andralex · 2017-07-08T11:22:12Z

std/net/curl.d

+            Throwable next = null)
+    {
+        this.status = status;
+        super(msg, file, line, next);


conventionally the call to super goes first in the ctor

andralex · 2017-07-08T11:22:26Z

std/net/curl.d

+        super(msg, file, line, next);
+    }
+
+    immutable int status; /// The HTTP status code


immutable -> nice!

- also see dlang/phobos#5551 for HTTPStatusException

- To make them distinguishable from (e.g. connection) errors and allow appropriate handling in client code.

MartinNowak · 2017-07-08T12:02:48Z

Done

andralex · 2017-07-08T12:28:30Z

std/net/curl.d

+/
+class HTTPStatusException : CurlException
+{
+    /++


eh, bug in -cov

andralex · 2017-07-08T12:28:39Z

std/net/curl.d

+            status = The HTTP status code.
+            msg  = The message for the exception.
+            file = The file where the exception occurred.
+            line = The line number where the exception occurred.


eh, another bug in -cov

Looks weird https://codecov.io/gh/dlang/phobos/compare/ecef6f0e3b3da14d483b3dd6e05d8780d16fd719...97e9db7f02ea072acb1a3f1bd11e3a1f3fca193a/changes, but if I run the single test locally dmd reports correct coverage.
It's also weird what we're doing the circleci script, looks like tests are supposed to run twice but fail to do so.
https://circleci.com/gh/dlang/phobos/3504 (no log line links in circleci :().

MartinNowak · 2017-07-08T18:30:17Z

Can we merge this now? Workaround for the broken coverage is done here #5579.

- and throw any other errors - also see dlang/phobos#5551 for HTTPStatusException

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Jul 5, 2017

handle expected 404 errors when fetching metadata

9aec0e0

- also see dlang/phobos#5551 for HTTPStatusException

wilzbach reviewed Jul 6, 2017

View reviewed changes

PetarKirov reviewed Jul 6, 2017

View reviewed changes

std/net/curl.d Outdated

super(msg, file, line, next);

}

int status; /// The HTTP status code

Copy link

Member

PetarKirov Jul 6, 2017

Choose a reason for hiding this comment

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

Make that const or immutable?

MartinNowak force-pushed the http_status_exception branch from 7d227d0 to 926b162 Compare July 7, 2017 22:26

MartinNowak mentioned this pull request Jul 7, 2017

download / registry requests silently fail dlang/dub#1104

Merged

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Jul 7, 2017

handle expected 404 errors when fetching metadata

2641a0f

- also see dlang/phobos#5551 for HTTPStatusException

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Jul 7, 2017

handle expected 404 errors when fetching metadata

91b84ea

- also see dlang/phobos#5551 for HTTPStatusException

PetarKirov approved these changes Jul 8, 2017

View reviewed changes

PetarKirov added the Merge:auto-merge label Jul 8, 2017

wilzbach removed the Merge:auto-merge label Jul 8, 2017

wilzbach assigned andralex Jul 8, 2017

wilzbach added the @andralex label Jul 8, 2017

andralex approved these changes Jul 8, 2017

View reviewed changes

andralex removed the @andralex label Jul 8, 2017

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Jul 8, 2017

handle expected 404 errors when fetching metadata

b6907f7

- also see dlang/phobos#5551 for HTTPStatusException

throw specific HTTPStatusException on http request errors

97e9db7

- To make them distinguishable from (e.g. connection) errors and allow appropriate handling in client code.

MartinNowak force-pushed the http_status_exception branch from 926b162 to 97e9db7 Compare July 8, 2017 12:02

andralex reviewed Jul 8, 2017

View reviewed changes

std/net/curl.d

+/

class HTTPStatusException : CurlException

{

/++

Copy link

Member

andralex Jul 8, 2017

Choose a reason for hiding this comment

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

eh, bug in -cov

andralex reviewed Jul 8, 2017

View reviewed changes

wilzbach added the Merge:auto-merge label Jul 8, 2017

dlang-bot merged commit 4cfe0f3 into dlang:master Jul 8, 2017

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Jul 9, 2017

handle expected 404 errors when fetching metadata

5c451fd

- and throw any other errors - also see dlang/phobos#5551 for HTTPStatusException

MartinNowak deleted the http_status_exception branch July 9, 2017 13:08

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Jul 10, 2017

handle expected 404 errors when fetching metadata

e605902

- and throw any other errors - also see dlang/phobos#5551 for HTTPStatusException

Uh oh!

Conversation

MartinNowak commented Jul 5, 2017

Uh oh!

dlang-bot commented Jul 5, 2017

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinNowak commented Jul 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinNowak commented Jul 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants