Skip to content

Conversation

@dgw
Copy link
Member

@dgw dgw commented Dec 2, 2022

Description

For #2381 I made sure we wouldn't introduce any Error-level alerts, because those would have given the master branch a ❌ check status. Anything below that level (that wasn't a false-positive or a test case) was mostly left alone.

This PR's branch name, useless-code, is inspired by the alert label that caught my eye first. At time of opening that label also applies to most of the alerts that will be fixed by this patch.

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 managed to break my flake8 somehow, but these tweaks are so minor I did not waste time trying to fix it just for this PR
  • I have tested the functionality of the things this change touches

dgw added 3 commits December 1, 2022 18:15
`_regex_type` isn't used here any more. It's moved on to bigger and
better places like the `loader` or `plugins.rules`.
These are technically part of the API, as constants. At least two core
plugins use them, so they shouldn't be made private.

This fixes two py/unused-global-variable alerts from CodeQL scanning.
@dgw dgw added the Housekeeping Code cleanup, removal of deprecated stuff, etc. label Dec 2, 2022
@dgw dgw added this to the 8.0.0 milestone Dec 2, 2022
@dgw dgw requested a review from a team December 2, 2022 18:20
Copy link
Contributor

@SnoopJ SnoopJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look fine to me. The commit message for 0b50255 should probably reference the CodeQL rule that it's fixing but you may plan to squash those references away anyhow.

I would like to formally declare my annoyance with CodeQL's py/import-and-import-from rule which looks more like someone's personal aesthetics than a serious concern about maintenance (what's the alternative supposed to be, import foo; # other imports; something = foo.something ?!)

@dgw
Copy link
Member Author

dgw commented Dec 3, 2022

The commit message for 0b50255 should probably reference the CodeQL rule that it's fixing

Yeah, I forgot that one, whoops. Fixed.

I would like to formally declare my annoyance with CodeQL's py/import-and-import-from rule which looks more like someone's personal aesthetics than a serious concern about maintenance (what's the alternative supposed to be, import foo; # other imports; something = foo.something ?!)

The alternative is supposed to be just using foo.something with the namespace, in your example case.

I personally agree with the idea that things should be imported either under a module namespace or individually, but not both. As a contrived demo, I think it would be silly to from sopel import plugin and then also from sopel.plugin import commands, and I think it would lead to potential confusion later.

But I also don't like importing individual module members in most cases anyway. I've found that the best way to have future-proof imports is always importing a (sub)module, and using that module's members as members (. notation). Then you 1) always know where something came from, because the module it's from is always included at the site of use; and 2) you can use other features of the module without modifying imports.

Because of (1) above, I also tend to avoid using import ____ as, except to avoid name collisions.

@dgw dgw merged commit 019eccc into master Dec 8, 2022
@dgw dgw deleted the useless-code branch December 8, 2022 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Housekeeping Code cleanup, removal of deprecated stuff, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants