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

plugins: command-regexp functions are now private #1944

Merged
merged 7 commits into from
Nov 6, 2020

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Sep 22, 2020

Description

Fix #1887

  • moved everything into sopel.plugins.rules, directly into each rule class
  • removed/deprecated a bunch of stuff from sopel.test_tools (this entire module should probably be deprecated and removed at some point anyway)
  • reworked imports in sopel.test_tools
  • changed how the example decorator runs a test (this should be transformed further, probably in a future PR, for 7.1 or 8.x)
  • removed obsolete tests and fixtures
  • made sure the doc is still OK

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 Tweak label Sep 22, 2020
@Exirel Exirel added this to the 7.1.0 milestone Sep 22, 2020
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.

I have to ask: Are all of these changes really related to the PR/commit title, plugins: command-regexp functions are now private? This is a rare occasion when I really think you're doing too much in one commit, unless all of the refactoring and such is truly necessary to do all at once. I'd say at least some of it probably isn't, and should be a separate commit or PR.

Two things about the overall approach here, though:

  1. The existing functions in tools should be marked deprecated until removal in 8.0, instead of yanked out in a point release with no warning. But nothing says we have to keep their implementation; what would you say to building the implementation inside Rule and its subclasses such that those tools functions would just call a classmethod or act as an accessor for the appropriate class constant?
  2. Can we put tools.compile_rule into plugins.rules too? Same thing about deprecation vs. outright removal in 7.1, but it has no reason to exist outside the rule system especially if the rule system is its primary user.

sopel/plugins/rules.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Sep 28, 2020

This is a rare occasion when I really think you're doing too much in one commit

I totally got carried away with that one, with a very bad case of tunnel vision. I'll see if I can separate that properly.

@Exirel Exirel force-pushed the plugins-private-regex-functions branch from 868d4e8 to 1f4df30 Compare October 2, 2020 09:18
@Exirel
Copy link
Contributor Author

Exirel commented Oct 2, 2020

I'll see if I can separate that properly.

Just did! I had to rebase everything, so I took that opportunity to fix the merge conflict as well.

@Exirel Exirel requested a review from dgw October 2, 2020 09:25
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.

Overall not a whole lot to say; mostly repeated suggestions for wording that's used in a half-dozen spots.

I complained about removing tests for the deprecated tools functions, but there was no place to leave a line note about making them just call the corresponding plugins.rules implementations until removal (what I suggested before).

sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Show resolved Hide resolved
test/test_tools.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
sopel/tools/__init__.py Outdated Show resolved Hide resolved
@dgw dgw force-pushed the plugins-private-regex-functions branch from 463346b to e4cd829 Compare October 11, 2020 20:43
@dgw
Copy link
Member

dgw commented Oct 11, 2020

@Exirel Pushed a couple commits to address my own feedback about test coverage and stuff. Instead of -0.1% coverage, the last PR build reported only -0.004%. 🏆

When it's time to straighten out the PR history, I suggest:

  • "fixup" the appropriate commit with the partial revert (currently 3176636)
  • either leave e4cd829 standalone or merge it with 1f4df30 (along with the other amendment, 452818c)

@Exirel
Copy link
Contributor Author

Exirel commented Oct 11, 2020

I'm really not fan of your solution here: when something is deprecated, it's for a reason. This should not be modified, and there is only one way to ensure that: don't change the code. Now, if we want to change the code in, say, the regex we use for good, we'll have to take care of deprecated functions we don't want to change.

@dgw
Copy link
Member

dgw commented Oct 11, 2020

These deprecated functions we don't want to change are meant to return the regex or pattern for the type of command they specify. The regex isn't going to change between now and 8.0, at which point these will be gone.

But let's say we do want to change something. We'd have made that change in these functions before; now we make it in plugins.rules instead. I don't see a difference. If someone's code is going to break because the pattern/regex returned by this tool function changed, it's going to break regardless of whether said pattern/regex came from tools/__init__.py or plugins/rules.py. 🤷‍♂️

@Exirel Exirel force-pushed the plugins-private-regex-functions branch from 45641b8 to 17b5f0e Compare October 12, 2020 09:55
@Exirel
Copy link
Contributor Author

Exirel commented Oct 12, 2020

@dgw rebased, keeping your changes. Should be good now.

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.

Extra tests for the regex args, too, it's magnificent!

sopel/plugins/rules.py Show resolved Hide resolved
Exirel and others added 7 commits October 26, 2020 15:45
Co-authored-by: dgw <dgw@technobabbl.es>
Co-authored-by: dgw <dgw@technobabbl.es>
We don't need to bother adding tests now for the `action_command` ones,
though. That is, unless someone's bored and wants to do it.
What it couldn't have in its prime, it now gets while on life support.
Thank you for your service, `tools.get_action_command_regex()`.
@Exirel Exirel force-pushed the plugins-private-regex-functions branch from 17b5f0e to 8174462 Compare October 26, 2020 14:48
@Exirel
Copy link
Contributor Author

Exirel commented Oct 26, 2020

Rebase because I want to work on the next step asap.

@dgw dgw merged commit a97fb64 into sopel-irc:master Nov 6, 2020
@Exirel Exirel deleted the plugins-private-regex-functions branch May 1, 2021 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tools: command-regexp functions shouldn't be public
2 participants