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

[REST] Reorganize error handling + fix issue #949 #1001

Merged
merged 1 commit into from Mar 12, 2015

Conversation

Projects
None yet
2 participants
@Geod24
Contributor

Geod24 commented Mar 7, 2015

A common problem I was encountering with REST was how painful it was to test failing cases (as the module should warn on interfaces error), as well as the duplication of code to have error both in client and server.
This centralize the error checking in a single CTFE function that is called from 2 static asserts. If the function returns non-null, it means there is an error (and the return is the error message).
Hence, it makes it quite easy to test issues, such as #949 which I originally intended to fix when I started this branch.

Note that static assert (ParamNames[i].length, ..) could never be false, as std.traits.ParameterIdentifierTuple would not compile (unless this has to do with an older release?). I fixed the test.
EDIT: Actually, this static assert now works in 2.067.

@Geod24 Geod24 changed the title from [REST] Reorganize error handling + fix issue 949 to [REST] Reorganize error handling + fix issue #949 Mar 8, 2015

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 9, 2015

Member

Interesting idea with the string return. I just think it looks a bit too clever like this. I'd at least change the name to something like getInterfaceValidationError, because reading static assert(!validateInterface!(I), validateInterface!(I)); had a slight WTF moment ;)

Maybe returning a small struct or tuple would be even cleaner (with the potential to safe one CTFE run), but the name change enough would be clean enough for me for an internal function.

Member

s-ludwig commented Mar 9, 2015

Interesting idea with the string return. I just think it looks a bit too clever like this. I'd at least change the name to something like getInterfaceValidationError, because reading static assert(!validateInterface!(I), validateInterface!(I)); had a slight WTF moment ;)

Maybe returning a small struct or tuple would be even cleaner (with the potential to safe one CTFE run), but the name change enough would be clean enough for me for an internal function.

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 9, 2015

Contributor

Interesting idea with the string return. I just think it looks a bit too clever like this. I'd at least change the name to something like getInterfaceValidationError, because reading static assert(!validateInterface!(I), validateInterface!(I)); had a slight WTF moment ;)

Maybe returning a small struct or tuple would be even cleaner (with the potential to safe one CTFE run), but the name change enough would be clean enough for me for an internal function.

It started as a template with a static assert inside it... But returning a message helps alot (because __traits(compiles, ...) is an awful way to check for correctness). And it needs to be a function because, well, CTFE and template are rarely compatible.

I changed the name, and add is null for extra verbosity. You're right, getInterfaceValidationError is much more suited as a name :)

Contributor

Geod24 commented Mar 9, 2015

Interesting idea with the string return. I just think it looks a bit too clever like this. I'd at least change the name to something like getInterfaceValidationError, because reading static assert(!validateInterface!(I), validateInterface!(I)); had a slight WTF moment ;)

Maybe returning a small struct or tuple would be even cleaner (with the potential to safe one CTFE run), but the name change enough would be clean enough for me for an internal function.

It started as a template with a static assert inside it... But returning a message helps alot (because __traits(compiles, ...) is an awful way to check for correctness). And it needs to be a function because, well, CTFE and template are rarely compatible.

I changed the name, and add is null for extra verbosity. You're right, getInterfaceValidationError is much more suited as a name :)

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 12, 2015

Contributor

Ping

Contributor

Geod24 commented Mar 12, 2015

Ping

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 12, 2015

Member

Okay, looks good. It's definitely an interesting idiom.

Just out of interest, why did you need __traits(compiles) with the template+static assert solution, but not for the CTFE string return version?

Member

s-ludwig commented Mar 12, 2015

Okay, looks good. It's definitely an interesting idiom.

Just out of interest, why did you need __traits(compiles) with the template+static assert solution, but not for the CTFE string return version?

@Geod24

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 12, 2015

Contributor

For testing.
If you put the assert directly inside the template, then how do you test that the generator correctly rejects invalid interface ? You're bound to use __traits(compiles), and that's an horrible solution because if it breaks for a different reason, the test will pass.
Check line ~1200 for an example.

Note that my first attempt was to write an external test, but those should usually be 'black box' testing, so it required me to duplicate every single test (1 for client and 1 for server).

Contributor

Geod24 commented Mar 12, 2015

For testing.
If you put the assert directly inside the template, then how do you test that the generator correctly rejects invalid interface ? You're bound to use __traits(compiles), and that's an horrible solution because if it breaks for a different reason, the test will pass.
Check line ~1200 for an example.

Note that my first attempt was to write an external test, but those should usually be 'black box' testing, so it required me to duplicate every single test (1 for client and 1 for server).

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 12, 2015

Member

Okay, makes sense.

Member

s-ludwig commented Mar 12, 2015

Okay, makes sense.

s-ludwig added a commit that referenced this pull request Mar 12, 2015

Merge pull request #1001 from Geod24/fix-949
[REST] Reorganize error handling + fix issue #949

@s-ludwig s-ludwig merged commit 8bf97b3 into vibe-d:master Mar 12, 2015

1 check passed

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

@Geod24 Geod24 deleted the Geod24:fix-949 branch Apr 27, 2015

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