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

feat: skipDoubleRegistration configuration #356

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

venkata-reddy-dev
Copy link
Contributor

@venkata-reddy-dev venkata-reddy-dev commented Mar 21, 2024

This PR is the implementation for feature request #335

Use case :

Some times developer don't want to touch production code to write test cases.

Scenario :

Code was release to production but some test cases are not written due to some high priorities.
if developer want to write test cases after some time, Then some test mock implementations need to put in place of real implementations.

To do this developer need to add if-else cases in production code to place right dependencies for test environments.
I feel touching production code after release, just for writing extra test cases is not a good approach.

if (testing) {
   getIt.registerSingleton<AppModel>(AppModelMock()); /// this mocks are in production code  
 } else {
   getIt.registerSingleton<AppModel>(AppModelImplementation());
 }

Currently get_it does not have a capability to skip registrations if already registered. it throws Argument Error

Here is the test case which throws Argument Error

test(' Throws ArgumentError ', () async {
   final getIt = GetIt.instance;
   getIt.allowReassignment = false;
   getIt.reset();
   getIt.registerSingleton<DataStore>(MockDataStore());

   expect(
     () => getIt.registerSingleton<DataStore>(RemoteDataStore()),
     throwsArgumentError,
   );
 }); 

This PR adds one boolean variable skipDoubleRegistration to get_it configuration to ignore registration silently.

Here is the test case:

test(' Ignores ReassignmentError ', () async {
    final getIt = GetIt.instance;
    getIt.reset();
    getIt.allowReassignment = false;
    getIt.skipDoubleRegistration = true;
    getIt.registerSingleton<DataStore>(MockDataStore());
    getIt.registerSingleton<DataStore>(RemoteDataStore()); ///  Just ignores registration silently 

    expect(getIt<DataStore>(), isA<MockDataStore>());
  });

To understand more, i have implemented a real world use case in a sample git repo, look into source code

I am open to take feedback and happy to modify this PR to land this feature requirement into get_it package.

@venkata-reddy-dev venkata-reddy-dev marked this pull request as draft March 21, 2024 05:59
@escamoteur
Copy link
Collaborator

But why not simply setting allowReasignment== true? why a new property? what do I miss here?

@escamoteur
Copy link
Collaborator

OK, I now understood the goal. We should name it skipDoubleRegistration and mark it visibleForTesting

@escamoteur
Copy link
Collaborator

Also please also add it to the readme

@venkata-reddy-dev
Copy link
Contributor Author

Okay I will do changes as requested

Thanks for the response

@venkata-reddy-dev venkata-reddy-dev changed the title feat: ignoreReassignmentError configuration feat: skipDoubleRegistration configuration Mar 23, 2024
@venkata-reddy-dev venkata-reddy-dev marked this pull request as ready for review March 23, 2024 13:46
@venkata-reddy-dev
Copy link
Contributor Author

@escamoteur changes are done as you suggested.

@venkata-reddy-dev
Copy link
Contributor Author

@escamoteur can you review this changes and merge, if there is no code review comments.

Copy link
Collaborator

@escamoteur escamoteur left a comment

Choose a reason for hiding this comment

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

Awesome work

@escamoteur escamoteur merged commit 29ab627 into fluttercommunity:master Apr 3, 2024
2 checks passed
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.

None yet

2 participants