qa: fix additional LGTM alerts #1754
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
So, exclusion comments don't work, either with or without a space between I'll come back to this another day. |
Exirel
requested changes
Nov 17, 2019
|
@Exirel Assuming the exclusion comments can be made to work, you're OK with this as amended? |
This comment has been minimized.
This comment has been minimized.
|
I'm not waiting 15 minutes for LGTM to re-analyze this with the suppression-comment changes dropped (I moved them to another branch; no reason to keep them unless they work). Presumably it'll pass the analysis with (still) 14 alerts fixed, and I'll merge it tomorrow afternoon-ish (currently 02:35 😅). |
LGTM pointed out that `requests` was imported with `from` as well. As long as I was in the file, I fixed the command docstrings.
LGTM pointed out that config.types.FilenameAttribute overrides two methods of ~.BaseValidated with different signatures, and complained that the overriden method's signature doesn't match places where it's called. Fair point. In a perfect world, we might make BaseValidated an abstract class. So doing is kind of a pain until we drop Python 2 support due to the THREE different syntaxes between py2.7, py3.3, and py3.4+... This appproach ought to hold us over until then.
The best constant to use was added only in Python 3.6, which makes addressing these warnings for Sopel 7.0 impractical. My lengthy comment should provide clear guidance for when we start going through the code looking for things to refactor after we actually drop old versions of Python, in (per current plans) Sopel 8.
It's kind of a bogus warning, since we aren't actually "sanitizing" anything here, but it certainly doesn't hurt to make sure we're looking at the *beginning* of the clbin result. If LGTM still flags this, I'll be mildly cross because `re.match()` only matches at the beginning of the given string. Technically, LGTM only flagged `post_to_clbin`, but since that and `post_to_0x0` are almost identical, I chose to hedge against them fixing whatever bug in their analysis engine caused it to catch only one.
LGTM says an object that defines `__eq__()` should also define both `__ne__()` and `__hash__()` for itself. That's easy, and it means we're now explicit about TimeReminder objects not being hashable.
Should silence LGTM's [py/procedure-return-value-used] warnings.
|
Right, so LGTM apparently doesn't apply suppression comments in PRs on purpose. I put them back, and will merge this Very Soon™. Gonna find something else to do while the analysis runs. 😁 |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since it seems running LGTM locally isn't a thing, I have to open this PR and hope that it does everything I think it should. I think this fixes 18 alerts. If the actual results don't match, I'll figure it out Soon™ (but probably not within the next 24 hours).
Edit: Looks like easy fixes (only the exception-comment changes failed, and it seems to be an inconsistency in LGTM's documentation about whether a space is allowed).