Skip to content
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

Allow multiple inheritance on the interface level with current restrictions intact #1341

Merged
merged 1 commit into from Jul 19, 2021

Conversation

silkentrance
Copy link
Contributor

fixes #1284

@silkentrance
Copy link
Contributor Author

@kdavisk6 still not getting a circleci detail link on the build... darn

@silkentrance
Copy link
Contributor Author

Tests are working just fine locally: https://gist.github.com/silkentrance/419a6a7e38032d0f2e33e4d0a46844c8

@kdavisk6
Copy link
Member

For some reason the webhook for your Pull request is being rejected by Circle CI. The hook response indicates a 400, but not sure why. Others have been able to push changes to their branches and have it work. We'll leave it for now.

@silkentrance
Copy link
Contributor Author

moving on and waiting for your input.

@MastaP
Copy link

MastaP commented Jan 28, 2021

Hi, any chance to get this PR fixed and merged?

@silkentrance
Copy link
Contributor Author

rebased to master, conflicts have been resolved

@silkentrance
Copy link
Contributor Author

@kdavisk6 the build fails on jdk8, complaining about a missing Path class from the javax.ws.rs package.

@kdavisk6
Copy link
Member

@silkentrance

Having read through this, I don't see anything that I feel the need to comment on. However, the changes are in our type handling and it will be very difficult to see what affect this will have on existing projects. It's a risky change, but I do see the value.

I looked over the build and it looks like there is something off with your fork. I recommend rebasing instead of merging with the latest master. Once the tests are green, we can look into merging.

@silkentrance
Copy link
Contributor Author

@kdavisk6 i did a rebase but there were some editing conflicts in the files i had been changing, so no biggie.

@silkentrance
Copy link
Contributor Author

I just did a new rebase. Let's see what circle ci has to say about this.

Building this locally with mvn -Pjava11,release[,windows,ossrh] clean compile test succeeded. The same is true for a build without the java11 profile enabled.

I am using openjdk 14.

[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Feign (Parent) 10.13-SNAPSHOT:
[INFO]
[INFO] Feign (Parent) ..................................... SUCCESS [  5.243 s]
[INFO] Feign Core ......................................... SUCCESS [ 40.522 s]
[INFO] Feign Gson ......................................... SUCCESS [  3.701 s]
[INFO] Feign JAX-RS ....................................... SUCCESS [  4.175 s]
[INFO] Feign Apache HttpClient ............................ SUCCESS [  4.400 s]
[INFO] Feign JAX-RS 2 ..................................... SUCCESS [  6.121 s]
[INFO] Feign Apache HttpComponents Client 5 ............... SUCCESS [  6.797 s]
[INFO] Feign Hystrix ...................................... SUCCESS [  6.938 s]
[INFO] Feign Jackson ...................................... SUCCESS [  4.570 s]
[INFO] Feign Jackson JAXB ................................. SUCCESS [  4.000 s]
[INFO] Feign JAXB ......................................... SUCCESS [  4.423 s]
[INFO] Feign OkHttp ....................................... SUCCESS [  4.098 s]
[INFO] Feign Google HTTP Client ........................... SUCCESS [  4.650 s]
[INFO] Feign Ribbon ....................................... SUCCESS [  7.422 s]
[INFO] Feign SAX .......................................... SUCCESS [  3.585 s]
[INFO] Feign SLF4J ........................................ SUCCESS [  3.325 s]
[INFO] Feign Mock ......................................... SUCCESS [  4.190 s]
[INFO] Feign spring ....................................... SUCCESS [  4.393 s]
[INFO] Feign SOAP ......................................... SUCCESS [  4.807 s]
[INFO] Feign Reactive Wrappers ............................ SUCCESS [  7.121 s]
[INFO] Feign Dropwizard Metrics4 .......................... SUCCESS [  3.679 s]
[INFO] Feign Dropwizard Metrics5 .......................... SUCCESS [  3.443 s]
[INFO] Feign Micrometer ................................... SUCCESS [  3.802 s]
[INFO] GitHub Example ..................................... SUCCESS [  2.088 s]
[INFO] Wikipedia Example .................................. SUCCESS [  2.106 s]
[INFO] Feign APT test generator ........................... SUCCESS [  6.026 s]
[INFO] Feign Benchmark (JMH) .............................. SUCCESS [  4.038 s]
[INFO] Feign Java 11 ...................................... SUCCESS [  2.905 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS

@silkentrance
Copy link
Contributor Author

@kdavisk6 again failing on jdk8.

I found this https://stackoverflow.com/questions/10869977/cant-find-javax-ws-rs-package-in-jdk which states that javax-ws-rs is not part of that jdk.

@silkentrance
Copy link
Contributor Author

just went through the hassle and installed openlogic openjdk 8 x64 for windows

$ /p/development/maven/bin/mvn -X -U -Prelease clean compile test
Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: P:\development\maven
Java version: 1.8.0-262, vendor: OpenLogic-OpenJDK, runtime: P:\development\jdks\openlogic-openjdk-8u262-b10-win-64\jre
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

and it also fails on the JAXRS2ContractTest.

@silkentrance
Copy link
Contributor Author

Retrying this with the master (with latest changes from upsteam) does not fail 😄.
I wonder what went wrong during the rebase to gh-1284.

@silkentrance
Copy link
Contributor Author

Okay, this was due to IntelliJ expanding the wildcard import from javax.ws.rs. I have reverted these changes and was able to eliminate changes to otherwise unrelated files.

@silkentrance
Copy link
Contributor Author

@kdavisk6 everything seems to be working now, see previous comment.

@silkentrance
Copy link
Contributor Author

rebased to master

@silkentrance
Copy link
Contributor Author

@kdavisk6 what about integrating this into the master branch and make this part of the forthcoming release?

Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

Seem Kevin was happy with the change, and so am I.

I agree can be risky, but, didn't conflict on any of my projects, so thumbs up

@velo velo merged commit 66de4f0 into OpenFeign:master Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE REQUEST:Multi Level Multiple Inheritance and Override Support
4 participants