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

Introduce SessionIdGenerator.DEFAULT #2399

Closed
wants to merge 1 commit into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Aug 18, 2023

organize code via centralizing the default SessionIdGenerator instance

organize code via centralizing the default SessionIdGenerator instance
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 18, 2023
@quaff
Copy link
Contributor Author

quaff commented Aug 18, 2023

It's safe to merge this before merging #2387. @marcusdacoregio

@marcusdacoregio marcusdacoregio self-assigned this Sep 1, 2023
@marcusdacoregio
Copy link
Contributor

Hi, @quaff. We do not want to merge this now because there are no clear pros to do it.

Also, adding the constant to the interface is a pattern that we do not follow and it is a bit weird that the interface has a direct reference to one of its implementations.

I will close this for now but we can always revisit when we get more feedback. Thank you!

@marcusdacoregio marcusdacoregio added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 1, 2023
@quaff
Copy link
Contributor Author

quaff commented Sep 2, 2023

Hi, @quaff. We do not want to merge this now because there are no clear pros to do it.

The pro is make code more organized, changes will be little if we switch to another default implementation.

Also, adding the constant to the interface is a pattern that we do not follow and it is a bit weird that the interface has a direct reference to one of its implementations.

Adding the constant to the interface is common pattern, for example:

https://github.com/spring-projects/spring-framework/blob/03650e7d58ebbd68e8cff22db379ec974b289423/spring-aop/src/main/java/org/springframework/aop/Pointcut.java#L51
https://github.com/spring-projects/spring-framework/blob/03650e7d58ebbd68e8cff22db379ec974b289423/spring-expression/src/main/java/org/springframework/expression/ParserContext.java#L62

or we could use static method instead of constant to take advantage of new feature of java language.

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.

3 participants