Skip to content

Conversation

@kezhevatov
Copy link
Contributor

GH-3169: Fix DefaultSessionFactoryLocator addSessionFactory key's type from String to Object

All other DefaultSessionFactoryLocator contracts are based on the Object, so this addSessionFactory has to be on Object as well.

@kezhevatov kezhevatov force-pushed the GH-3169 branch 2 times, most recently from 2bdca46 to 0d0c0be Compare February 6, 2020 20:53
@kezhevatov kezhevatov requested a review from artembilan February 6, 2020 20:58
*/
@Deprecated
public void addSessionFactory(String key, SessionFactory<F> factory) {
this.factories.put(key, factory);
Copy link
Member

Choose a reason for hiding this comment

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

I have an opinion that deprecated API should call a new one for better coverage.
This way when we need to make a change in the target logic, a deprecated API won't be affected because it is meant to be for removal eventually anyway. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it make sense. Done

…ctory key's type from String to Object

All other DefaultSessionFactoryLocator contracts are based on the Object, so this addSessionFactory has to be on Object as well.
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Merging...

I'll add your name to the class author list and do some clean up locally...

@artembilan
Copy link
Member

Merged as edf84a3 and cherry-picked to 5.2.x.

@kezhevatov ,

thank you for the contribution; looking forward for more!

For the future, please, consider to test the project against your changes: you have missed to fix a deprecated method usage in the DelegatingSessionFactoryTests.

Plus it would be great to make a good commit message (not a PR description, but commit): https://chris.beams.io/posts/git-commit/

@artembilan artembilan closed this Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants