Skip to content

Conversation

@dgw
Copy link
Member

@dgw dgw commented Nov 4, 2022

Description

The connection_registered attribute itself is now a property, which first checks to see if the bot's backend exists (is not None) and then whether the backend.is_connected(), before finally seeing if the internal _connection_registered flag has been set.

Also added a few places where the internal flag should be reset back to its initial value of False, such as if the bot is about to quit or the backend's on_close() handler was called.

All of this is to try and make bot.connection_registered reliable for plugins like remind, which have scheduled tasks that might need to send data to IRC and need a reliable way to check whether the connection is ready to accept such data before operating.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches
    • Note that this required adding a bit more setup to two test_coretasks units.

The `connection_registered` attribute itself is now a property, which
first checks to see if the bot's `backend` exists (is not `None`) and
then whether the `backend.is_connected()`, before finally seeing if the
internal `_connection_registered` flag has been set.

Also added a few places where the internal flag should be reset back to
its initial value of `False`, such as if the bot is about to quit or the
backend's `on_close()` handler was called.

All of this is to try and make `bot.connection_registered` reliable for
plugins like `remind`, which have scheduled tasks that might need to
send data to IRC and need a reliable way to check whether the connection
is ready to accept such data before operating.
@dgw dgw added Tweak Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels Nov 4, 2022
@dgw dgw added this to the 8.0.0 milestone Nov 4, 2022
@dgw dgw requested a review from a team November 4, 2022 23:22
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

That will do.

I wonder if a Threading.Event could be used for _connection_registered instead of a boolean, but that's a nitpick and not necessary right now.

@dgw dgw merged commit a911266 into master Dec 4, 2022
@dgw dgw deleted the bot.connection_registered-property branch December 4, 2022 19:22
@dgw
Copy link
Member Author

dgw commented Dec 4, 2022

Threading.Event could be used for _connection_registered instead of a boolean

This is a neat idea and worth looking into, largely because of the .wait() method that would make available.

@dgw dgw added the Breaking Change Stuff that probably should be mentioned in a migration guide label Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking Change Stuff that probably should be mentioned in a migration guide Bugfix Generally, PRs that reference (and fix) one or more issue(s) Tweak

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants