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

plugin: type-hint priority argument as Literal[] #2555

Merged
merged 2 commits into from
Nov 19, 2023
Merged

plugin: type-hint priority argument as Literal[] #2555

merged 2 commits into from
Nov 19, 2023

Conversation

dgw
Copy link
Member

@dgw dgw commented Nov 11, 2023

Description

This will flag any use of unrecognized values for the callable priority. It makes a good intermediate step between the current free-form str accepted and probably turning it into an Enum later.

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 lint and make test)
  • I have tested the functionality of the things this change touches
    • Indeed, if I change one of the priority decorators in core to an unlisted value, mypy raises an error 👍

@dgw
plugin: type-hint priority argument as Literal[]
7676a7a 
This will flag any use of unrecognized values for the callable priority.
It makes a good intermediate step between the current free-form `str`
accepted and probably turning it into an `Enum` later.
@dgw dgw added the Tweak label Nov 11, 2023
@dgw dgw added this to the 8.0.0 milestone Nov 11, 2023
@dgw dgw requested a review from a team November 11, 2023 20:02
SnoopJ
SnoopJ approved these changes Nov 11, 2023
View reviewed changes
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.

If you want to file an issue about making an Enum now (or if we already have one, I didn't check), it would be helpful to add in a # TODO above the change, but otherwise LGTM

sopel/plugin.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member Author

dgw commented Nov 11, 2023

@SnoopJ you can look at the revised docstring if/when you like. I'll go try to finish that Enum issue I started before our big conversation on IRC about other PRs started. 😁

@dgw dgw mentioned this pull request Nov 11, 2023
Open
4 tasks
@Exirel Exirel self-requested a review November 12, 2023 00:10
@dgw dgw force-pushed the priority-literal branch from c2891ee to 6c14f50 Compare November 13, 2023 04:03
@dgw
Copy link
Member Author

dgw commented Nov 13, 2023

OK, amended. You can do that final review you self-requested at your leisure, @Exirel 😃

Exirel
Exirel approved these changes Nov 15, 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 well. I guess this is a fight I can't win. I don't care anymore. Go with it.

sopel/plugin.py Outdated Show resolved Hide resolved
@dgw dgw force-pushed the priority-literal branch from 6c14f50 to 1196b19 Compare November 15, 2023 20:05
@dgw
Copy link
Member Author

dgw commented Nov 15, 2023

Oh well. I guess this is a fight I can't win. I don't care anymore. Go with it.

If this is about the quotes you added a line note to, that was an error in my rebase. Should be fixed. 😅

@dgw dgw merged commit 179ab82 into master Nov 19, 2023
8 checks passed
@dgw dgw deleted the priority-literal branch November 19, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SnoopJ SnoopJ SnoopJ approved these changes

@Exirel Exirel Exirel approved these changes

Assignees
No one assigned
Labels
Tweak
Projects
None yet
Milestone
8.0.0
Development

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