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

polishing FeignClientsRegistrar #1038

Merged

Conversation

birariro
Copy link
Contributor

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

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Hello @birariro, I'm not sure why you've proposed this change. We do need to use beanFactory to properly resolve the attributes.

@birariro
Copy link
Contributor Author

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
Copy link
Member

Keep the for testing comment

@OlgaMaciaszek
Copy link
Collaborator

OlgaMaciaszek commented Jun 26, 2024

@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.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks @birariro. Looks good. Can you please just add your full name and surname with @author tag to the javadoc of the class and update the date in the license comment to 2013-2024?

@birariro
Copy link
Contributor Author

@OlgaMaciaszek I have applied the changes as requested.

@OlgaMaciaszek OlgaMaciaszek added this to the 4.2.0 milestone Sep 26, 2024
@OlgaMaciaszek OlgaMaciaszek merged commit 1c00226 into spring-cloud:main Sep 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants