Fix for #85 #92
Fix for #85 #92
Conversation
|
feign-pull-requests #121 SUCCESS |
core/src/main/java/feign/Util.java
Outdated
There was a problem hiding this comment.
I don't think this can be null in context.
|
note that this implementation and suggestion assume that the value is a whole string that contains only the variable. We should add a comment noting this. Ex. |
|
thanks for following this through, @wnagele! |
|
Ps dont forget to update CHANGES |
|
feign-pull-requests #128 FAILURE |
|
Worked in the feedback that I could. Failure from Cloudbees is cause this can't be auto-merged anymore. |
|
Anything else you want me to do before merging this? |
|
Same thing as with my other pull request. Silence and it's definitely a bug that this would fix. Not very encouraging to contribute to this project. |
|
@wnagele you need to rebase this against upstream master so that it can be merged cleanly. that's all this has needed since you updated the read me. |
|
I have now rebased this against the current master. |
|
feign-pull-requests #137 ABORTED |
|
@wnagele im not sure when we started enforcing checkstyle, but an irrelevant change is toasting the build. Can anyone raise a PR to fix slf4? Cc @allenxwang @jdamick |
|
Can checkstyle be disabled if there is not value in it? |
|
Unless I'm misreading the build output, it looks to me like the failure was due to the configuration of the build timeout plugin, not checkstyle rules being enforce. The build state is ABORTED, with a message of "Build timed out (after 20 minutes). Marking the build as aborted." From the build time trend there are various other builds at the 18-20 minute mark that appear to have been close to being aborted with this constraint. https://netflixoss.ci.cloudbees.com/job/feign-pull-requests/137/console |
|
@davidmc24 I have changed the timeout to be 30 minutes. |
|
feign-pull-requests #138 SUCCESS |
|
Re-pushed. Looks good now. Ready for merge. |
|
@davidmc24 @allenxwang thanks for fixing the build! @wnagele thanks for the improvement |
|
This doesn't seem to be included in the 6.1.1 release. |
5 participants
No description provided.