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

bot: new setup sequence #1597

Merged
merged 3 commits into from
Jul 20, 2019
Merged

bot: new setup sequence #1597

merged 3 commits into from
Jul 20, 2019

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented May 11, 2019

At first, I had to rework the Job Scheduler in order to test how Sopel registers and unregisters plugins, which was annoying. But then, the solution wasn't perfect: I didn't like to have to manually stop the scheduler for testing purpose—which is time consuming and a bad thing when your test suite grow a bit (a lot at the moment). Aside from that, it also means that plugins where loaded, even if I didn't want them to be, or even needed to be. This is the first thing I fixed, by moving the call to Sopel.setup() outside of Sopel.__init__().

Then, I realized: wait a minute... why do we setup plugins inside Sopel, but not logging? I couldn't find a good reason, so I moved that into Sopel's setup sequence.

Eventually, I thought: why do I even bother with the scheduler? Why do we need it started before anything else is even loaded? Answer is: we don't. We don't need that, and we don't have to start it whenever we instantiate Sopel.

So here we go:

  • setup logging
  • setup plugins
  • start the job scheduler

all wrapped nicely into the old but still relevant Sopel.setup() method.

BONUS POINT: now the tests are slightly faster, and it's one step closer to facilitate reusability of sopel.bot.Sopel class for testing purpose (and everything else).

@Exirel Exirel added this to the 7.0.0 milestone May 11, 2019
@Exirel Exirel requested a review from dgw May 11, 2019 17:02
@HumorBaby
Copy link
Contributor

... and it's one step closer to facilitate reusability of sopel.bot.Sopel class for testing purpose

❤️

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I have a nitpick, as usual, but I like how much this simplifies the tests. 👍

sopel/bot.py Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Jul 5, 2019

What's wrong with you, Scrutinizer?

@dgw
Copy link
Member

dgw commented Jul 5, 2019

Scrutinizer is just a flake.

@Exirel
Copy link
Contributor Author

Exirel commented Jul 9, 2019

I had to rebase because of a conflict. :(

@Exirel Exirel requested a review from dgw July 9, 2019 21:30
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I think this is good to go, once rebased again to fix another merge conflict that crept in.

Once this goes in, I will take care of the follow-up in #1655.

Exirel added 3 commits July 20, 2019 20:26
Sopel's setup sequence is now:

* setup logging
* setup plugins
* start Job Scheduler

There are two major change:

* Job Scheduler is now started after setup is complete
* Logging setup is now called from `Sopel.setup`

There is no good reason for the Job Scheduler to be started before the
bot is ready to run, since jobs can be safely added or removed while
it's not running. One may even consider that we should start the Job
Sceduler once the bot is connected only - but that's for another day.
@Exirel
Copy link
Contributor Author

Exirel commented Jul 20, 2019

I had to rebase because of a conflict. :(

Again.

@dgw dgw merged commit 85ff9d6 into sopel-irc:master Jul 20, 2019
@Exirel Exirel deleted the init-setup-run branch September 5, 2019 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants