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

help: replace with dependency on sopel-help #2332

Merged
merged 1 commit into from
Aug 21, 2022

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Jul 23, 2022

Description

Fix #1212 by proxy.

Make the built-in help plugin obsolete with warning in its setup() hook and all rules have been removed (rules & commands), and installing Sopel now requires sopel-help.

Since it is possible to install a package without its dependencies, I kept help.py so users can have a warning. It isn't much, but it might help someone.

This is based of #2342 for Python 3.10.

Thank you for your service, help, you will be remembered.

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 Breaking Change Stuff that probably should be mentioned in a migration guide Housekeeping Code cleanup, removal of deprecated stuff, etc. labels Jul 23, 2022
@Exirel Exirel added this to the 8.0.0 milestone Jul 23, 2022
@Exirel Exirel mentioned this pull request Jul 23, 2022
40 tasks
@dgw
Copy link
Member

dgw commented Jul 24, 2022

Are the internals that help/sopel-help use well-enough documented that someone could easily make a custom equivalent? (Not talking about adding e.g. paste backends to sopel-help, but an actual full-on replacement.)

Otherwise, I'm 👎 on replacing an essential built-in plugin like this with a no-op. Moving to package it separately (so it can be updated independently of core) is fine, but core should require it.

@Exirel
Copy link
Contributor Author

Exirel commented Jul 24, 2022

Are the internals that help/sopel-help use well-enough documented that someone could easily make a custom equivalent?

Not yet! This is in the work with Sopel 8, by replacing the hard-coded dict and stuffs like that by the Rule Managers, which could be the origin of a new chapter in "advanced tips & tricks" in the documentation.

core should require it.

Alright then! Waiting on #2328 first, then I'll add it to the list of install-requires. 😁

@Exirel Exirel marked this pull request as ready for review August 14, 2022 21:05
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.

You know, the irony here is that I was ready to merge this despite the use of "module" because it's unlikely that most users would ever see it. But because you rebased it onto another PR that isn't merged yet, I have to wait, and so do you. 😝

(If you wait until #2342 goes in before amending this one, the UI will be less confusing. The extra commits will disappear after you force-push, but only if they've been merged.)

sopel/modules/help.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member

dgw commented Aug 15, 2022

Oh, I just had another (possibly stupid) idea: Could help.py be moved to some other place called "shims" or similar, so we have a place for this and other plugins that are moved out of core but still want to keep around with warnings like this for a bit?

Obviously I know it can, but is the idea good? 😛

@dgw
Copy link
Member

dgw commented Aug 21, 2022

Wait is over; #2342 has been merged. Amend away, and soon ye shall receive another merge. 😁

The help built-in plugin is obsolete, and replaced by
sopel-help which can be installed with pip install sopel-help (as a
standalone install) or when installing Sopel through pip.

A warning has been added to the sopel.modules.help.setup() in case
someone install Sopel without its dependencies.
All rules/commands have been removed.

Thank you for your service, help, you will be remembered.

Co-authored-by: dgw <dgw@technobabbl.es>
@Exirel
Copy link
Contributor Author

Exirel commented Aug 21, 2022

Done. I had to fix the message, and decided to keep a single \' because I could have that or use double quotes and use two \" for the last line.

@Exirel Exirel requested a review from dgw August 21, 2022 19:45
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.

and decided to keep a single \' because I could have that or use double quotes and use two \" for the last line.

Maybe you forgot that the below totally works 😁 but it's totally a choice between two almost equally annoying alternatives (mixed quotes vs. escapes). The third option would be using double quotes and wrapping the command in something else, maybe backticks.

Anyway, I'm already over it.

>>> print(
...     "Sopel's built-in help plugin is obsolete. "
...     'Install sopel-help as the official help plugin for Sopel.\n'
...     'You can install sopel-help with "pip install sopel-help".')
Sopel's built-in help plugin is obsolete. Install sopel-help as the official help plugin for Sopel.
You can install sopel-help with "pip install sopel-help".

@dgw dgw changed the title help: remove all rules from help help: replace with dependency on sopel-help Aug 21, 2022
@dgw dgw merged commit a4f185d into sopel-irc:master Aug 21, 2022
@Exirel
Copy link
Contributor Author

Exirel commented Aug 21, 2022

I did not. I remembered that you didn't like that syntax on my attempt at using flake8-quotes, mixing " and ' in multi-lines.

@dgw
Copy link
Member

dgw commented Aug 21, 2022

Well, I don't. But I also don't like escapes. But this is also almost never going to be seen by anyone, until its eventual deletion in 9.0 or whatever, so again: I'm over it. 😁 😹

@Exirel Exirel deleted the help-is-deprecated branch April 8, 2023 22:22
@dgw dgw mentioned this pull request Apr 19, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Stuff that probably should be mentioned in a migration guide Housekeeping Code cleanup, removal of deprecated stuff, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

help: using a pastebin should be optional
2 participants