-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add conjure endpoint errors #1670
base: master
Are you sure you want to change the base?
Conversation
Generate changelog in
|
e03492f
to
e9c67be
Compare
conjure-core/src/main/java/com/palantir/conjure/defs/EndpointErrorResolver.java
Outdated
Show resolved
Hide resolved
conjure-core/src/main/java/com/palantir/conjure/parser/services/EndpointError.java
Outdated
Show resolved
Hide resolved
e9c67be
to
f353903
Compare
f353903
to
caffb0e
Compare
Back to draft, realized I didn't update the documentation. |
conjure-core/src/main/java/com/palantir/conjure/parser/services/EndpointError.java
Outdated
Show resolved
Hide resolved
b560b95
to
6f4ab77
Compare
7c06435
to
2c1f0ce
Compare
2c1f0ce
to
872ce14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's cut an rc, nice work!
Invalidated by push of 853ff93
853ff93
to
46d15a8
Compare
docs: optional<Documentation> | ||
deprecated: optional<Documentation> | ||
markers: list<Type> | ||
tags: set<string> | ||
EndpointName: | ||
alias: string | ||
docs: Should be in lowerCamelCase. | ||
EndpointError: | ||
fields: | ||
error: TypeName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think TypeName may be a bit misleading here, there's a fair bit of overlap, but I think perhaps we should flatten the TypeName
components (package + name) into this type, and name it ErrorTypeName
505d890
to
e334bee
Compare
record ErrorResolutionResult( | ||
String errorName, String package_, com.palantir.conjure.spec.ErrorNamespace namespace) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to use the new ErrorTypeName type here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Updated.
docs: optional<Documentation> | ||
deprecated: optional<Documentation> | ||
markers: list<Type> | ||
tags: set<string> | ||
EndpointName: | ||
alias: string | ||
docs: Should be in lowerCamelCase. | ||
EndpointError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this to ErrorTypeName
? It's basically equivalent to TypeName
, but for errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry: this type makes sense, but should encapsulate a pair of ErrorTypeName (package/namespace/name) + docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
8b70e0a
to
b7189de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
[skip ci]
Invalidated by push of d0d10d1
Before this PR
Expand the Conjure specification to support specifying errors that can be thrown on each endpoint.
After this PR
==COMMIT_MSG==
==COMMIT_MSG==
Possible downsides?