Skip to content

Comments

Fixes #2711 Do not register GroupRepository by default#2712

Merged
msevestre merged 2 commits intoV13from
2711-do-not-register-grouprepository-by-default
Nov 26, 2025
Merged

Fixes #2711 Do not register GroupRepository by default#2712
msevestre merged 2 commits intoV13from
2711-do-not-register-grouprepository-by-default

Conversation

@rwmcintosh
Copy link
Member

Fixes #2711

Description

Each endpoint application has unique requirements for GroupRepository. Each has to make sure to register the correct implementation

Type of change

Please mark relevant options with an x in the brackets.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires documentation changes (link at least one user or developer documentation issue):
  • Algorithm update - updates algorithm documentation/questions/answers etc.
  • Other (please describe):

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Integration tests
  • Unit tests
  • Manual tests
  • No tests required

Reviewer checklist

Mark everything that needs to be checked before merging the PR.

  • Check if the code is well documented
  • Check if the behavior is what is expected
  • Check if the code is well tested
  • Check if the code is readable and well formatted
  • Additional checks (document below if any)
  • Check if documentation update issue(s) are created if the option This change requires a documentation update above is selected

Screenshots (if appropriate):

Questions (if appropriate):

@rwmcintosh rwmcintosh self-assigned this Nov 26, 2025
Copilot AI review requested due to automatic review settings November 26, 2025 16:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses issue #2711 by removing the default registration of GroupRepository from the CLIRegister class and instead requiring each endpoint application to explicitly register its own implementation. The changes ensure that applications have control over which GroupRepository implementation to use, with a fallback to a default implementation when no custom registration action is provided.

Key Changes

  • Removed the default singleton registration of IGroupRepository from CLIRegister
  • Added conditional registration logic in ApplicationStartup.Initialize() to register a default GroupRepository only when no custom registerAction is provided
  • Added a test to verify the correct registration behavior

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/OSPSuite.CLI.Core/CLIRegister.cs Removed default singleton registration of IGroupRepository; moved exclusion comment for clarity
src/OSPSuite.R/Bootstrap/ApplicationStartup.cs Added conditional logic to register default GroupRepository when no custom registerAction is provided
tests/OSPSuite.R.Tests/Services/SimulationPersisterSpecs.cs Added test to verify correct GroupRepository registration; removed unused System import

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@msevestre msevestre left a comment

Choose a reason for hiding this comment

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

I don't understand the weird if stuff. Maybe we need some comment in ther code OR the reg needs to be done seomwhere else. It feels a bit out of place

using (container.OptimizeDependencyResolution())
{
if (registerAction != null)
if (registerAction == null)
Copy link
Member

Choose a reason for hiding this comment

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

this looks weird to me without explanation. Why do we have this weird "if" to register the group repository. I can't tell

Copy link
Member Author

Choose a reason for hiding this comment

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

it is weird. I'll comment

Copy link
Member Author

Choose a reason for hiding this comment

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

The jist of it is that when MoBi.R initializes the OSPSuite.R it uses this to do some of its own registration. I am using this as a signal that someone else is responsible for the GroupRepository. PK-Sim does not use this convention at all.

Maybe we should do this completely different for MoBi though. I think we did it this way to re-use this Initialize and then add the MoBi stuff on top without copy/pasting the code

Copy link
Member

Choose a reason for hiding this comment

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

yeah . We have no idea if this is the case or not. It's a requirement of any application using this do actually do it. Interesting. I think this is a smell but a small one

@rwmcintosh rwmcintosh requested a review from msevestre November 26, 2025 17:55
@msevestre msevestre merged commit 7314061 into V13 Nov 26, 2025
1 check passed
@msevestre msevestre deleted the 2711-do-not-register-grouprepository-by-default branch November 26, 2025 18:00
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.

3 participants