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

remind: fix import streamlining gone haywire #2470

Merged
merged 1 commit into from
Jun 14, 2023
Merged

Conversation

dgw
Copy link
Member

@dgw dgw commented Jun 12, 2023

"Unexpected AttributeError: 'str' object has no attribute 'utc'" begone.

Fixes a bug I introduced in #2468 by not being careful enough with my imports. Code review or not, I should've caught it. (And I was reminded to finish/open this PR by mypy --check-untyped-defs flagging these errors, so that's all the more reason to work toward #2461 in the longer term.)

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

mypy --check-untyped-defs still reports errors in sopel/modules/remind.py after this patch, but they'll be fixed separately. @SnoopJ and I have been chipping away at that list over the weekend.

"Unexpected AttributeError: 'str' object has no attribute 'utc'" begone.
@dgw dgw added High Priority Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels Jun 12, 2023
@dgw dgw added this to the 8.0.0 milestone Jun 12, 2023
@dgw dgw requested a review from a team June 12, 2023 19:06
Copy link
Contributor

@SnoopJ SnoopJ left a comment

Choose a reason for hiding this comment

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

Changes look fine to me, although this would also be a good opportunity to add a test that would have caught the bug

@dgw
Copy link
Member Author

dgw commented Jun 13, 2023

this would also be a good opportunity to add a test that would have caught the bug

Went for the minimum-necessary-change approach because:

  1. As discovered shortly after, mypy --check-untyped-defs (the new default behavior of make mypy, soon) flags this problem
  2. More importantly, the not-so-long-term plan is to replace this built-in plugin with an external sopel-remind package that already exists and has much better test coverage

Basically, I was lazy in favor of just getting it working again so our master-based bot unbreaks. Once the new package has optional-date support for .at, we can just drop the builtin.

@dgw dgw merged commit bc5843b into master Jun 14, 2023
@dgw dgw deleted the remind-timezone-str branch June 14, 2023 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) High Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants