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

loader, plugin, rules: add rules_match() decorator #2288

Closed
wants to merge 1 commit into from

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented May 28, 2022

Description

Fix #462 with the last option: have a rules_match (name suggested by @dgw) decorator with a flag for RAW/PLAIN/BOTH.

  • add a new decorator sopel.plugin.rules_match
  • add a flag enum sopel.formatting.MatchText: the enum is required in various modules (plugin, plugins.rules, loader, etc.) and it fits the theme of the plain function
  • add a note in plugin documentation about how rules are using the raw version by default
  • updated AbstractRule.parse from getting only text to text, plain, making it a bit awkward to use
  • base Rule and SearchRule were easy to deal with; FindRule required a bit of trickery, which means that you can't partially match both version, if you find anything in one, the other is ignored (when using the match BOTH)

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

@Exirel Exirel added the Feature label May 28, 2022
@Exirel Exirel added this to the 8.0.0 milestone May 28, 2022
@Exirel
Copy link
Contributor Author

Exirel commented Jun 5, 2022

There was a conflict and no review at the moment so I took the chance, rebased, and now I'll ask review. 😛

@Exirel Exirel requested a review from dgw June 5, 2022 17:10
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.

Haven't looked at tests yet, since I asked to change some names and those will change the related tests too if you accept the ideas.

You can ignore my lament about the direction this took vs. what was initially planned in #462, if you want. I don't quite understand all the pain points related to find and search, but I suspect at least the lazy ones have to do with "oops, we already shipped this public interface that would have to change" and........ yeah, OK. That would be a pain, yes. And if not all places where rules can be defined could support a new kwarg, then it doesn't make sense to be inconsistent.

docs/source/plugin/anatomy.rst Outdated Show resolved Hide resolved
sopel/formatting.py Outdated Show resolved Hide resolved
sopel/plugin.py Outdated Show resolved Hide resolved
@dgw dgw removed this from the 8.0.0 milestone Jul 13, 2022
@dgw
Copy link
Member

dgw commented Jul 13, 2022

Letting @Exirel take the last few steps here, but per discussion in this thread + on IRC, we are postponing this feature until the next version in favor of reworking some innards to make it (and other ideas like it) work better. The current plan is to close this PR, and build a new one on top of it later.

@dgw dgw added the Replaced Superseded by a newer PR label Jul 13, 2022
Co-authored-by: dgw <dgw@technobabbl.es>
@Exirel Exirel closed this Jul 13, 2022
@Exirel Exirel deleted the rule-skip-control-code branch April 8, 2023 22:20
@Exirel Exirel restored the rule-skip-control-code branch April 8, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Replaced Superseded by a newer PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stripping control codes
2 participants