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

Remove cruft from modules #1402

Merged
merged 42 commits into from
Apr 17, 2019
Merged

Remove cruft from modules #1402

merged 42 commits into from
Apr 17, 2019

Conversation

dgw
Copy link
Member

@dgw dgw commented Oct 28, 2018

What started as a fixup in #1400 (noticing that a regex string literal I corrected to pass new style checks was actually unused) turned into a longer-term project to go through all the modules and remove old, orphaned cruft that isn't needed any more.

The version module is the first to receive this attention, and I'll check the rest after Hacktoberfest ends.

@dgw dgw self-assigned this Oct 28, 2018
@dgw dgw modified the milestones: 7.0.0, 6.6.0 Nov 2, 2018
@dgw dgw modified the milestones: 6.6.0, 7.0.0 Jan 3, 2019
@dgw
Copy link
Member Author

dgw commented Jan 3, 2019

Pushed back to 7.0.0, so I can focus on the last remaining "more important" fixes for 6.6.0.

@dgw dgw mentioned this pull request Apr 16, 2019
dgw and others added 3 commits April 16, 2019 05:16
The `log_line` regex was orphaned in ab05f11 and hasn't been used since.
That was almost five years ago. No `re` stuff needed in the module now.
Actually, it's from 6 years ago now. — dgw, rebasing onto current master

Co-Authored-By: dgw <dgw@technobabbl.es>
@dgw dgw changed the title [WIP] Remove cruft from modules Remove cruft from modules Apr 16, 2019
dgw added 21 commits April 16, 2019 05:23
Unified style of header with the other modules. Had to guess at year of
authorship by browsing to the Phenny repo (but the module might be older
even than that).

Also added a couple more punctuation options for `hello()` to use.
On top of updating the comment pointing to the pronoun.is JSON API pull
request we'd like to see merged (the old one was merged into a feature
branch), this also fixes some typos and handles the example commands
properly using the help prefix set in the bot's config.

Oh, and it also removed an unused global logger. You know, actual cruft.
Use myself (current maintainer) as example, instead of old maintainer
who probably doesn't want to be bothered (or highlighted in case someone
happens to get that help output in a channel where they are).

Sort imports, rename global constant to UPPERCASE, and add a bit of
future-thinking documentation for in-progress work.
Renamed memory key from "uptime" to "start_time", since it's actually
storing the bot's start time and not its uptime.

Tweaked some other code/docstring style, too.
Prettified file header, added missing docstring(s), etc.
Overhauled docstrings and sorted imports while looking for unused code.
At some point in the future, we want to enforce good import order.
Standard imports before modules, and external packages before our own,
that kind of thing. Since I'm going through all the modules anyway,
sorting the imports makes sense to reduce the workload later.
Overhauled import order and docstrings while looking for unused code.
A blank line between standard and module imports is nice. Not enforced
(yet), but nice to have.

Also, since we're using UTF-8 for this source file, why did we not
output a Unicode '…' ellipsis instead of '...' three periods? We do now.
I came looking for cruft, but the closest thing I found was outdated
comments referencing "google" even though we haven't used Google for
years. (It was DuckDuckGo for a while, but now it's Bing, because DDG
broke using multiple `site:` queries and never fixed the bug.)

So, now we just talk vaguely about searching the web, instead of Google.

We also have a nicer module-level docstring, which is part of the
cruft-cleaning project as well.
@dgw
Copy link
Member Author

dgw commented Apr 16, 2019

This is now ready for eyes. There are a lot of little changes, so take your time!

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.

Yes, a thousand times yes!

@Exirel
Copy link
Contributor

Exirel commented Apr 17, 2019

@dgw just 🚢 it 😉

@dgw dgw merged commit 59d217b into sopel-irc:master Apr 17, 2019
@dgw dgw deleted the cleanup-module-cruft branch April 17, 2019 07:11
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.

3 participants