search

scientistproject / Scientist.net

A .NET library for carefully refactoring critical paths. It's a port of GitHub's Ruby Scientist library
MIT License
1.46k stars 95 forks source link

Set Enabled in StaticHelper to afect all instances #129

Open marioanogueira opened 5 years ago

marioanogueira commented 5 years ago

Background for this PR. Having a few experiments running without using the Static Helper, was trying to disable all experiments at once, by using the Enabled method. Turns out it wasn't possible.

To achieve this Had to create a implementation of Scientist and override the IsEnabledAsync.

This PR aims to allow the set of the Enabled func globally, for experiments using the helper and for the implementations. The IsEnabledAsync remains virtual so that we can still override the enabled behaviour for a specific one

M-Zuber commented 5 years ago

I am not sure how the changes accomplish the stated goal, could you maybe provide some code samples to show the difference in use between the current code, and the code you are proposing?

marioanogueira commented 5 years ago

Sure, so right now we have two ways of starting an Experiment, either by using the Static helper like Experiment<bool>(ExperimentName, experiment => { experiment.Use(() => true); experiment.Try(() => false); }); or by creating a new Scientist object like this var scientist = new Scientist(resultPublisher); scientist.Experiment<bool>("", experiment => { experiment.Use(() => true); experiment.Try(() => false); }); Given this, if we have multiple experiments on both formats and we want to use the Enabled function to disable or enable all of the experiments the current implementation doesn't allow it. The current Enabled function only affects experiments created using the static helper.

My changes allow the Enabled to affect all of the esperiments regardless if they are being create using the static helper or the instance

M-Zuber commented 5 years ago

One change that could be helpful in evaluating this would be to revert the naming change of _sharedScientist to SharedScientist

Do you think you could write a test that will only pass with this change?

marioanogueira commented 5 years ago

Yeah sure, I'll write the test and update the PR.

Will revert the name change also

M-Zuber commented 5 years ago

Thank you! (also, you might to keep this in mind: https://github.com/scientistproject/Scientist.net/pull/108#issuecomment-390773865)

marioanogueira commented 5 years ago

I do have a few more ideas to make it more IoC friendly, but I wanna do them in a different PR. My changes won't affect the current friendliness of IoC/DI, in fact it will improve since it will allow to disable experiments that are being created by a IoC container

marioanogueira commented 5 years ago

@M-Zuber did you had time to review the latest changes?