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

Empty optional exception mapper #1784

Merged
merged 2 commits into from Oct 25, 2016

Conversation

Projects
None yet
3 participants
@nickbabcock
Contributor

nickbabcock commented Oct 23, 2016

Another implementation of #1732, but using exceptionmappers, which now have a lower code tax due to #1768. By default, backwards compatibility is preserved with the ability to easily remap it to a 204.

The shape of the custom exception (EmptyOptionalException) inspired by netty's ReadTimeoutException to be a lightweight exception, as 204's may be a normal state and shouldn't be treated exceptionally.

Maybe controversial, but I've decided to add another submodule, dropwizard-e2e (end-to-end). It's like dropwizard-example except that it is meant to hold multiple Applications, as it's possible that a single example application can't represent all options especially if some are conflicting. It also presents another avenue for tests that ensure unit + integration tests work. If others want to verify it looks correct, it'd be much appreciated.

Closes #1727

cc original author: @qinfchen

I can update the release notes once this is merged.

@coveralls

This comment has been minimized.

coveralls commented Oct 23, 2016

Coverage Status

Coverage increased (+0.01%) to 82.31% when pulling a5161f9 on nickbabcock:empty-optional-exception into 394773c on dropwizard:master.

@nickbabcock nickbabcock force-pushed the nickbabcock:empty-optional-exception branch from a5161f9 to 69128c1 Oct 23, 2016

@coveralls

This comment has been minimized.

coveralls commented Oct 23, 2016

Coverage Status

Coverage increased (+0.01%) to 82.31% when pulling 69128c1 on nickbabcock:empty-optional-exception into 394773c on dropwizard:master.

@jplock jplock added the improvement label Oct 23, 2016

@jplock jplock added this to the 1.1.0 milestone Oct 23, 2016

@jplock

This comment has been minimized.

Member

jplock commented Oct 23, 2016

I like the idea of the dropwizard-e2e module. I'm wondering if we should standardize on some naming conventions though instead of a generic App1? Are there any other tests floating around that could be integrated into this new module as well (or anything that we haven't been able to test thoroughly in the past)?

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Oct 24, 2016

I know for #1768, dropwizard-e2e would have been the preferred test implementation because it is a much more realistic integration test.

I'm fine if we can find a theme for each application. Might have to ponder possible themes.

@jplock

This comment has been minimized.

Member

jplock commented Oct 25, 2016

I think we can merge this for now and refactor the tests later.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Oct 25, 2016

Updated release notes in 00cc42f

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