search

spring-cloud / spring-cloud-openfeign

Support for using OpenFeign in Spring Cloud apps
Apache License 2.0
1.2k stars 779 forks source link

polishing FeignClientsRegistrar #1038

Closed birariro closed 1 week ago

birariro commented 3 months ago

getName (Map<String, Object>) does not need to be for test purposes.

birariro commented 3 months ago

Hello @OlgaMaciaszek

String getName(ConfigurableBeanFactory beanFactory, Map<String, Object> attributes) Because the only way to call is String getName(Map<String, Object> attributes) Isn't beanFactory always null?

And /* for testing */ String getName(Map<String, Object> attributes) If you look at the comments, it says it's for testing, but if it's unnecessary, it looked better without it

spencergibb commented 3 months ago

Keep the for testing comment

OlgaMaciaszek commented 3 months ago

@birariro It's helpful to know that the visibility level has been raised or sth has been added for testing purposes. That's why we put these comments there, but actually in this case, if we remove the other overloaded method, it should be Visible for testing. Feel free to change to that. I thought getName was also called at a different point, but possibly after some refactoring it isn't, so right, we can change that.

birariro commented 1 week ago

@OlgaMaciaszek I have applied the changes as requested.