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

core: add __all__ statements in sopel fix #765 #1582

Merged
merged 5 commits into from
Jul 6, 2019

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented May 1, 2019

The __all__ statement in a Python module is used at import time when from module import * is used to expose only a defined list of object to be imported in the locals() environment of the importer.

Even though the Python documentation talks about "sub-modules" to import, it is widely used to define a set of names to import instead (being objects, functions, or sub-modules).

@Exirel Exirel added this to the 7.0.0 milestone May 1, 2019
@Exirel Exirel requested a review from a team May 1, 2019 15:53
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 made some easy tweaks. Adding documentation is a bit more than just adding __all__ statements, but I'm not going to complain about adding docstrings, like ever!

sopel/bot.py Outdated Show resolved Hide resolved
sopel/web.py Outdated Show resolved Hide resolved
sopel/bot.py Outdated Show resolved Hide resolved
sopel/bot.py Outdated Show resolved Hide resolved
sopel/bot.py Outdated Show resolved Hide resolved
sopel/bot.py Outdated Show resolved Hide resolved
sopel/bot.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member

dgw commented May 11, 2019

@Exirel I test-rendered the Sphinx docs to make sure your space-less display<target> method links worked as expected, and now I'm wondering if you would object to shortening some of the display names on parameter types. Like, showing sopel (sopel.bot.Sopel) is pretty verbose for that parameter to SopelWrapper, when it's even in the same module.

Also, I found a stray "arround" that I missed when reviewing earlier. :/

@Exirel
Copy link
Contributor Author

Exirel commented May 11, 2019

showing sopel (sopel.bot.Sopel) is pretty verbose for that parameter to SopelWrapper

Oh yes, you're right. I don't object, quite the contrary, it's a good idea.

Also, I found a stray "arround" that I missed when reviewing earlier. :/

Will fix that.

@Exirel
Copy link
Contributor Author

Exirel commented May 11, 2019

@dgw I added a commit where I added ~ in front of sopel.bot.Sopel, and I tried something with the overridden methods.

@Exirel Exirel removed the request for review from a team June 29, 2019 15:56
@dgw
Copy link
Member

dgw commented Jul 1, 2019

Also, I found a stray "arround" that I missed when reviewing earlier. :/

Will fix that.

It's still there. :/

"""Wrapper arround a Sopel instance and a Trigger

Aside from that, I'm ready to ✔️ and :shipit:.

@Exirel
Copy link
Contributor Author

Exirel commented Jul 2, 2019

Aside from that, I'm ready to heavy_check_mark and :shipit:.

I forgot, sorry. Should be good now. https://github.com/sopel-irc/sopel/pull/1582/files#diff-1cf03588484fddc0a6c0f9600a2d330cR869

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.

Yep, it's good now. :shipit:

@Exirel Exirel mentioned this pull request Jul 6, 2019
@dgw dgw mentioned this pull request Jul 6, 2019
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'm glad I gave this one more look. We could have broken some plugins that were doing from sopel.formatting import * and then trying to use colors.NAMEs.

sopel/formatting.py Show resolved Hide resolved
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.

Re-approved ✔️

Any further issues that slipped by will be caught by some brave soul running Sopel from source. 😁

@dgw dgw merged commit f390668 into sopel-irc:master Jul 6, 2019
@Exirel Exirel deleted the define-module-all branch September 5, 2019 09:44
This pull request was closed.
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