close
The Wayback Machine - https://web.archive.org/web/20201226184614/https://github.com/github/scientist/pull/104
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

Simplify configuration with Scientist::Experiment.register convenience method #104

Merged
merged 3 commits into from Sep 8, 2020

Conversation

@rlue
Copy link
Contributor

@rlue rlue commented Jun 26, 2019

I thought it was kind of clumsy to have to override Scientist::Experiment.new when configuring Scientist, so I created a Scientist::Experiment.register method to reduce the last five lines of configuration down to one.

In the second commit on this PR, I automate the registration of new experiments via a self.included callback, meaning that last line of configuration can be removed entirely.

I believe these changes should be fully backward-compatible with the previous API, since the original approach was to manually override Scientist::Experiment.new anyway.

@rlue rlue changed the title Add Scientist::Experiment.register convenience method Simplify configuration with Scientist::Experiment.register convenience method Jun 26, 2019
@coveralls
Copy link

@coveralls coveralls commented Jun 26, 2019

Coverage Status

Coverage increased (+0.03%) to 99.119% when pulling e9abd04 on rlue:api/register_method into 25a22f7 on github:master.

2 similar comments
@coveralls
Copy link

@coveralls coveralls commented Jun 26, 2019

Coverage Status

Coverage increased (+0.03%) to 99.119% when pulling e9abd04 on rlue:api/register_method into 25a22f7 on github:master.

@coveralls
Copy link

@coveralls coveralls commented Jun 26, 2019

Coverage Status

Coverage increased (+0.03%) to 99.119% when pulling e9abd04 on rlue:api/register_method into 25a22f7 on github:master.

@coveralls
Copy link

@coveralls coveralls commented Jun 26, 2019

Coverage Status

Coverage increased (+0.03%) to 99.119% when pulling 66823c9 on rlue:api/register_method into 25a22f7 on github:master.

@rlue rlue force-pushed the rlue:api/register_method branch from e9abd04 to dcaeaa1 Jun 27, 2019
@rlue rlue force-pushed the rlue:api/register_method branch from dcaeaa1 to 98833c2 Jun 27, 2019
@rlue
Copy link
Contributor Author

@rlue rlue commented Jun 27, 2019

Ninja edit: Fixed CI failures and rewrote commit messages.

Note that two out of four CI failures were caused by the auto-configuration "magic" I introduced in the second commit of this PR. This meant that in order to get the tests to pass, I had to add a line to the test setup to revert that "magic".

If the maintainers find this change of behavior unacceptable, I'd be happy to roll this PR back by one commit.

@jejacks0n
Copy link
Contributor

@jejacks0n jejacks0n commented Aug 9, 2020

I would like to see this as well, especially with the readme updates with the named arg corrections.

@rlue
Copy link
Contributor Author

@rlue rlue commented Sep 3, 2020

Anything I can do to help get this merged?

@zerowidth
Copy link
Member

@zerowidth zerowidth commented Sep 4, 2020

I like the idea of a class method to set the default experiment rather than overriding new. What do you think about set_default(klass) instead of register(klass)?register feels like adding something to a registry, implying multiple items, whereas set_default is more explicit.

I was initially concerned about automatically registering upon module inclusion since it could lead to require-time ordering issues if there are multiple base experiments. Since Scientist.run is only ever expecting a single implementation, though, that should be fine.

@rlue
Copy link
Contributor Author

@rlue rlue commented Sep 6, 2020

Good point; I'm on board!

@zerowidth zerowidth merged commit ba46e31 into github:master Sep 8, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zerowidth
Copy link
Member

@zerowidth zerowidth commented Sep 8, 2020

Thanks! I've released this as 1.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.