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.

Sign up for GitHub

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

Jump to bottom

core: better bot-mode handling #2448

Merged
merged 8 commits into from
May 28, 2023
Merged

core: better bot-mode handling #2448

merged 8 commits into from
May 28, 2023

Conversation

dgw
Copy link
Member

@dgw dgw commented Apr 29, 2023

Description

Sending mode +B by default is pretty silly. As of #2088 we should have stopped doing so. Now we will.

More importantly, WHO replies contain a bot flag on servers that support the bot-mode spec; it's indicated by the same character as the modechar advertised in ISupport's BOT token. We should track that—and you guessed it: this patch adds an is_bot attribute to User objects.

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

Notes

I'm hoping to write some more tests when I have time, because this part of coretasks seems pretty under-tested. Therefore, I'm starting the patch as a draft, to collect feedback until I get to that step.

@dgw dgw added this to the 8.0.0 milestone Apr 29, 2023
@dgw
Copy link
Member Author

dgw commented Apr 29, 2023

Ditto #2447 (comment) (but since this is still a draft, it's not as annoying… yet).

Exirel
Exirel reviewed Apr 29, 2023
View reviewed changes
Copy link
Contributor

@Exirel Exirel left a comment •
edited by dgw
Loading

Choose a reason for hiding this comment

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

Minor nitpicking. I wonder if you could add type annotations to _record_who as well (and run make mypy to ensure it actually works).

sopel/tools/target.py Outdated Show resolved Hide resolved
sopel/config/core_section.py Show resolved Hide resolved
@dgw
Copy link
Member Author

dgw commented Apr 30, 2023

Status update: Located an issue where several people are discussing the Coveralls error that the CI jobs are running into on this pull request. See lemurheavy/coveralls-public#1708

@dgw
Copy link
Member Author

dgw commented May 1, 2023

CI should pass again when this is rebased: lemurheavy/coveralls-public#1710 (comment)

Exirel
Exirel requested changes May 3, 2023
View reviewed changes
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.

Oh, nice test! Only nitpicking left, and we should be good.

sopel/config/core_section.py Outdated Show resolved Hide resolved
sopel/tools/target.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member Author

dgw commented May 3, 2023

Running make mypy on what I'm about to push (including new hints on _record_who() as suggested) returned:

$ make mypy
mypy sopel
sopel/plugins/capabilities.py:64: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/plugins/capabilities.py:70: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/plugins/capabilities.py:71: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/plugins/capabilities.py:72: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/irc/backends.py:59: error: Invalid type: try using Literal[False] instead?  [valid-type]
sopel/plugins/rules.py:1015: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/plugins/rules.py:1016: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/plugins/rules.py:1145: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/plugins/rules.py:1151: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/plugins/rules.py:1196: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/plugins/rules.py:1198: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/bot.py:56: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
sopel/modules/url.py:472: error: Incompatible types in "yield" (actual type "Tuple[str, str, Optional[str], Optional[str], bool]", expected type "URLInfo")  [misc]
Found 2 errors in 2 files (checked 86 source files)
make: *** [Makefile:9: mypy] Error 1

Of the two errors in these results, sopel/irc/backends.py:59 is fixed in #2430 (ready to merge) and sopel/modules/url.py:472 is fixed in #2443 (currently still a draft). Seems we're all good for types on stuff that's already annotated.

The [annotation-unchecked] stuff is a big can of worms and unrelated to this specific PR—but running mypy sopel --check-untyped-defs raises several dozen errors. That's for a future PR to deal with, maybe for 8.0, maybe not. (Whenever we get to it: Some interesting cases get flagged, sure to raise at least one of @Exirel's eyebrows…and possibly his blood pressure, too. 😜)

@dgw dgw marked this pull request as ready for review May 3, 2023 19:27
@dgw dgw requested a review from Exirel May 3, 2023 19:27
@dgw
Copy link
Member Author

dgw commented May 3, 2023 •
edited
Loading

The bot mode handling is already fully tested, actually, and I'm not sure what other tests I wanted to write. Hence, I've marked the PR as ready for review.

I remembered what I wanted to test: the WHO handling. I couldn't find any test cases that would trigger recv_who(), so I wrote a couple and added them to this PR.

@Exirel Exirel mentioned this pull request May 6, 2023
Merged
4 tasks
dgw added 8 commits May 24, 2023 02:06
@dgw
core_section: remove default B mode from modes setting
9e8a01d 
We shouldn't automatically send a nonstandard modechar by default. The
BOT ISupport token should be all we handle out of the box; if someone
wants to send a non-standard bot mode on a network that doesn't
advertise the BOT token, they can configure it themselves.
@dgw
tools.target: add is_bot attribute to User object
a5de5b1
@dgw
coretasks: update User.is_bot attribute if possible from WHO replies
8c54861
@dgw
tools.target: finish type-hinting module
e82c94f 
Converts `User.hostmask` from a lambda to a proper function, in order to
apply the intended type hints. (Setting `hostmask: str = property(...)`
generated an error, of course. It was also not possible to type-hint the
lambda's return value without stooping to `typing.Callable[]`, and I
figured I might as well just make the thing a real function.)

TODO comment added regarding user/host values that haven't populated yet
and are still set to the default of `None`.
@dgw
docs: note automatic use of bot-mode + alternative modes setting
d654a21
@dgw
coretasks: type-hint _record_who()
7feeaf5 
No related errors are reported by mypy, so it should be good to go.
@dgw
tools.target: better docstrings about User attrs that can be None
ab9969f
@dgw
tests: basic test cases for coretasks.recv_who()
e90317d
@dgw
Copy link
Member Author

dgw commented May 24, 2023

Finally got around to squashing down the fixups. 💪

@dgw dgw merged commit 74b8d4d into master May 28, 2023
@dgw dgw deleted the bot-mode-flag branch May 28, 2023 17:16
@dgw dgw mentioned this pull request Sep 24, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Exirel Exirel Exirel approved these changes

Assignees
No one assigned
Labels
Core/IRC Protocol Handling Tweak
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dgw @Exirel