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

[Core] Remove Database::singleton function #8427

Merged
merged 17 commits into from
Mar 8, 2023
Merged

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Mar 7, 2023

There are currently 3 methods of getting a reference to a database in LORIS, in order of preference:

  1. LorisInstance->getDatabaseConnection()
  2. NDB_Factory::singleton()->database();
  3. Database::singleton()

The only reference to 3 left is in the implementation of option 2. This removes the Database::singleton by inlineing the reference into the factory and removes all remaining references to Database::singleton (mostly from documentation and a couple unit tests.)

@driusan driusan added the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Mar 7, 2023
ridz1208
ridz1208 previously approved these changes Mar 8, 2023
Copy link
Collaborator

@ridz1208 ridz1208 left a comment

Choose a reason for hiding this comment

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

looks good other than the couple comments

@driusan driusan added Cleanup PR or issue introducing/requiring at least one clean-up operation and removed "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help labels Mar 8, 2023
@driusan
Copy link
Collaborator Author

driusan commented Mar 8, 2023

@kongtiaowang it seems like some unit tests can't connect to the database but the log has the credentials and it's consistently the same tests.. do you have any ideas?

@driusan driusan added the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Mar 8, 2023
@driusan driusan removed the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Mar 8, 2023
@driusan driusan requested a review from ridz1208 March 8, 2023 16:06
@driusan
Copy link
Collaborator Author

driusan commented Mar 8, 2023

@ridz1208 getting the tests to pass took more significant changes than expected, can you re-review?

@driusan driusan merged commit 6b80e4f into aces:main Mar 8, 2023
@ridz1208 ridz1208 added this to the 25.0.0 milestone Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants