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: support multiple examples #1403

Merged
merged 3 commits into from
Apr 20, 2019
Merged

help: support multiple examples #1403

merged 3 commits into from
Apr 20, 2019

Conversation

dgw
Copy link
Member

@dgw dgw commented Nov 7, 2018

Should behave the same as before for modules that don't use the new parameter to @sopel.module.example() to declare multiple help examples.

Updated admin.py module as a proof of concept. Going through other modules and updating them with multiple examples is a separate project.

Output is still tweakable, but I'm happy enough without worrying about splitting lines just yet.


This is WIP code I based off of #1303. Between the length of time between when I last looked at this and the present, and the fact that I had to manually rebase it on master, I'm definitely leaving it labeled WIP until I get back to working on it (probably after 6.6.0 releases).

Because this changes the module API (a new argument to @sopel.module.example() is introduced), this will be a Sopel 7 project.

@dgw dgw added this to the 7.0.0 milestone Nov 7, 2018
@dgw dgw self-assigned this Nov 7, 2018
@dgw
Copy link
Member Author

dgw commented Mar 28, 2019

Lots of changes since I touched this last! Felt good to add tests for it, though.

@dgw dgw requested a review from Exirel March 28, 2019 21:02
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.

It looks good. I didn't test it myself, but I'm so glad you were able to update and add unit-tests for that change!

As written in my comments, I'm asking for in-line comment, because I wasn't able to really understand what each line does. It sounds good, but I feel like a few words could prevent confusion.

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

dgw commented Mar 30, 2019

@Exirel We're turning the usual back-and-forth on its head! It's my turn to push new commits for you to 👍 before I squash them into the appropriate places in the PR's history. 😁

@dgw dgw requested a review from Exirel April 16, 2019 07:05
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.

LGTM

dgw added 3 commits April 17, 2019 02:35
Should behave the same as before for modules that don't use the new
parameter to @sopel.module.example() to declare multiple help examples.

Updated admin.py module as a proof of concept. Going through other
modules and updating them with multiple examples is a separate project.

Output is still tweakable, but I'm happy enough without worrying about
splitting lines just yet.
The loader didn't *have* tests when work on supporting multiple help
examples started. Rebasing was fun because of that.
Test the new `user_help` argument, making sure examples are included or
excluded properly based on its value.
@dgw dgw merged commit eb23031 into sopel-irc:master Apr 20, 2019
@dgw dgw deleted the 1200-multiple-examples branch April 20, 2019 13:07
@dgw dgw mentioned this pull request Apr 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants