Skip to content

Reduce visibility of mapper implementations #315

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

Closed
wants to merge 1 commit into from

Conversation

sjohnr
Copy link
Member

@sjohnr sjohnr commented Jun 17, 2021

Polish gh-304

@sjohnr
Copy link
Member Author

sjohnr commented Jun 17, 2021

@jgrandja @ovidiupopa91 - Looking at @rlewczuk's PR #291, those mappers are private, and I was thinking it seems like a better starting point than public. Any thoughts?

@ghost
Copy link

ghost commented Jun 18, 2021

Hi @sjohnr . For me making the mappers private makes sense.

The main reason why I left them public is that some time ago when I submitted a PR in spring-security for R2dbcReactiveOAuth2AuthorizedClientService I received a comment saying that I should declare the mappers as public not private.

But I don't remember the exact reason. I'm not sure if the reason was to follow the approach from JdbcOauth2AuthorizedClientService (where the mappers are declared as public as well) or something else.

@sjohnr
Copy link
Member Author

sjohnr commented Jun 18, 2021

Thanks for the clarification @ovidiupopa91. Just trying to figure out which way we want to go with the initial release of these components, which is why this is just a draft (idea). Will keep you posted as we discuss more.

@sjohnr sjohnr closed this Jun 22, 2021
@sjohnr sjohnr deleted the gh-304-polish branch June 22, 2021 15:45
@sjohnr sjohnr self-assigned this Jun 29, 2021
@sjohnr sjohnr added status: declined A suggestion or change that we don't feel we should currently apply type: refactoring labels Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants