8000 Allow HystrixFeign.Builder to be used incrementally. by spencergibb · Pull Request #313 · OpenFeign/feign · GitHub
[go: up one dir, main page]
Skip to content

Allow HystrixFeign.Builder to be used incrementally. #313

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.

Sign up for GitHub

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

Jump to bottom
Closed
spencergibb wants to merge 1 commit into from
Closed

Allow HystrixFeign.Builder to be used incrementally. #313

spencergibb wants to merge 1 commit into from

Conversation

@spencergibb
Copy link
Contributor
@spencergibb spencergibb commented Jan 13, 2016

No description provided.

spencergibb
spencergibb reviewed Jan 13, 2016
View reviewed changes
hystrix/README.md
Copy link
Contributor Author
@spencergibb spencergibb Jan 13, 2016

Choose a reason for hiding this comment

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

Because the change is in a terminal method you need it's original type 😞. One option is to move create a fallback() method which could retain the fluent style. Another is to create an AbstractBuilder<B> that returns B from each method allowing downstream builders to have custom terminal methods.

@cloudbees-pull-request-builder
Copy link
cloudbees-pull-request-builder commented Jan 13, 2016

NetflixOSS » feign » feign-pull-requests #200 SUCCESS
This pull request looks good

spencergibb
spencergibb reviewed Jan 13, 2016
View reviewed changes
hystrix/src/main/java/feign/hystrix/HystrixFeign.java
Copy link
Contributor Author
@spencergibb spencergibb Jan 13, 2016

Choose a reason for hiding this comment

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

Since the method can no longer be hidden, what do you think should be done here?

Copy link
Contributor
@codefromthecrypt codefromthecrypt Jan 14, 2016

Choose a reason for hiding this comment

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

unsupportedoperationexception

@codefromthecrypt
Copy link
Contributor
codefromthecrypt commented Jan 14, 2016

I had this in my intellij history, so yeah :P

codefromthecrypt pushed a commit that referenced this pull request Jan 14, 2016
Allows HystrixFeign.Builder to be used incrementally
9f3424e 
Uses covariant overrides so that those collecting Feign configuration
via `Feign.Builder` can cast into `HystrixFeign.builder` to target an
api.

Closes #313
@codefromthecrypt
Copy link
Contributor
codefromthecrypt commented Jan 14, 2016

ps the feeling against a base builder<X extends builder> is more-or-less that this became unwieldy in other projects (like jclouds). For example a type doing a static method now has an awkward Builder<?> which makes overrides even weirder to resolve. I don't like covariance either, just it contains the complexity as an internal detail which users won't see.

@spencergibb spencergibb deleted the hystrix-builder-reuse branch January 14, 2016 17:08
velo pushed a commit that referenced this pull request Oct 8, 2024
Allows HystrixFeign.Builder to be used incrementally
9f91100 
Uses covariant overrides so that those collecting Feign configuration
via `Feign.Builder` can cast into `HystrixFeign.builder` to target an
api.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@spencergibb @cloudbees-pull-request-builder @codefromthecrypt
0